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

John Ogness
2019-08-21T05:43:09Z

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