Re: comments style: Re: [RFC PATCH v4 1/9] printk-rb: add a new printk ringbuffer implementation

[John Ogness <john.ogness@linutronix.de>]

On 2019-08-20, Petr Mladek <pmladek@suse.com> wrote:
>> --- /dev/null
>> +++ b/kernel/printk/dataring.c
>> +/**
>> + * _datablock_valid() - Check if given positions yield a valid data block.
>> + *
>> + * @dr:         The associated data ringbuffer.
>> + *
>> + * @head_lpos:  The newest data logical position.
>> + *
>> + * @tail_lpos:  The oldest data logical position.
>> + *
>> + * @begin_lpos: The beginning logical position of the data block to check.
>> + *
>> + * @next_lpos:  The logical position of the next adjacent data block.
>> + *              This value is used to identify the end of the data block.
>> + *
>
> Please remove the empty lines between arguments description. They make
> the comments too scattered.

Your feedback is contradicting what PeterZ requested[0]. Particularly
when multiple lines are involved with a description, I find the spacing
helpful. I've grown to like the spacing, but I won't fight for it.

>> +	/*
>> +	 * dB:
>> +	 *
>> +	 * When a writer has completed accessing its data block, it sets the
>> +	 * @id thus making the data block available for invalidation. This
>> +	 * _acquire() ensures that this task sees all data ringbuffer and
>> +	 * descriptor values seen by the writer as @id was set. This is
>> +	 * necessary to ensure that the data block can be correctly identified
>> +	 * as valid (i.e. @begin_lpos, @next_lpos, @head_lpos are at least the
>> +	 * values seen by that writer, which yielded a valid data block at
>> +	 * that time). It is not enough to rely on the address dependency of
>> +	 * @desc to @id because @head_lpos is not depedent on @id. This pairs
>> +	 * with the _release() in dataring_datablock_setid().
>
> This human readable description is really useful.
>
>> +	 *
>> +	 * Memory barrier involvement:
>> +	 *
>> +	 * If dB reads from gA, then dC reads from fG.
>> +	 * If dB reads from gA, then dD reads from fH.
>> +	 * If dB reads from gA, then dE reads from fE.
>> +	 *
>> +	 * Note that if dB reads from gA, then dC cannot read from fC.
>> +	 * Note that if dB reads from gA, then dD cannot read from fD.
>> +	 *
>> +	 * Relies on:
>> +	 *
>> +	 * RELEASE from fG to gA
>> +	 *    matching
>> +	 * ADDRESS DEP. from dB to dC
>> +	 *
>> +	 * RELEASE from fH to gA
>> +	 *    matching
>> +	 * ADDRESS DEP. from dB to dD
>> +	 *
>> +	 * RELEASE from fE to gA
>> +	 *    matching
>> +	 * ACQUIRE from dB to dE
>> +	 */
>
> But I am not sure how much this is useful.

When I was first implementing RFCv3, the "human-readable" text version
was very useful for me. However, now it is the formal descriptions that
I find more useful. They provide the proof and a far more detailed
description.

> It would take ages to decrypt all these shortcuts (signs) and
> translate them into something human readable. Also it might get
> outdated easily.
>
> That said, I haven't found yet if there was a system in all
> the shortcuts. I mean if they can be descrypted easily
> out of head. Also I am not familiar with the notation
> of the dependencies.

I'll respond to this part in Sergey's followup post.

> If this is really needed then I am really scared of some barriers
> that guard too many things. This one is a good example.
>
>> +	desc = dr->getdesc(smp_load_acquire(&db->id), dr->getdesc_arg);

The variable's value (in this case db->id) is doing the guarding. The
barriers ensure that db->id is read first (and set last).

>> +
>> +	/* dD: */
>
> It would be great if all these shortcuts (signs) are followed with
> something human readable. Few words might be enough.

I'll respond to this part in Sergey's followup post.

>> +	next_lpos = READ_ONCE(desc->next_lpos);
>> +
>> +	if (!_datablock_valid(dr,
>> +			      /* dE: */
>> +			      atomic_long_read(&dr->head_lpos),
>> +			      tail_lpos, begin_lpos, next_lpos)) {
>> +		/* Another task has already invalidated the data block. */
>> +		goto out;
>> +	}
>> +
>> +
>> +++ b/kernel/printk/numlist.c
>> +bool numlist_read(struct numlist *nl, unsigned long id, unsigned long *seq,
>> +		  unsigned long *next_id)
>> +
>> +	struct nl_node *n;
>> +
>> +	n = nl->node(id, nl->node_arg);
>> +	if (!n)
>> +		return false;
>> +
>> +	if (seq) {
>> +		/*
>> +		 * aA:
>> +		 *
>> +		 * Adresss dependency on @id.
>> +		 */
>
> This is too scattered. If we really need so many shortcuts (signs)
> then we should find a better style. The following looks perfectly
> fine to me:
>
> 		/* aA: Adresss dependency on @id. */

I'll respond to this part in Sergey's followup post.

>> +		*seq = READ_ONCE(n->seq);
>> +	}
>> +
>> +	if (next_id) {
>> +		/*
>> +		 * aB:
>> +		 *
>> +		 * Adresss dependency on @id.
>> +		 */
>> +		*next_id = READ_ONCE(n->next_id);
>> +	}
>> +

John Ogness

[0] https://lkml.kernel.org/r/20190618111215.GO3436@hirez.programming.kicks-ass.net