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

[John Ogness]

On 2019-08-20, Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> wrote:
> [..]
>> > +	 *
>> > +	 * 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. 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.
>
> Does not appear to be systematic to me, but maybe I'm missing something
> obvious. For chains like
>
> 		jA->cD->hA to jB
>
> I haven't found anything better than just git grep jA kernel/printk/
> so far.

I really struggled to find a way to label the code in order to document
the memory barriers. By grepping on "jA:" you will land at the exact
location.

> But once you'll grep for label cD, for instance, you'd see
> that it's not defined. It's mentioned but not defined
>
> 	kernel/printk/ringbuffer.c:      * jA->cD->hA.
> 	kernel/printk/ringbuffer.c:      * RELEASE from jA->cD->hA to jB

I tried to be very careful about the labeling, but you just found an
error. cD is supposed to be cC. (I probably refactored the labels and
missed this one.) Particularly with referencing labels from other files
I was not happy (which is the case with cC). This is one area that I
think it would be really helpful if the kernel guidelines had some
format.

The labels are necessary for the technical documentation of the
barriers. And, after spending much time in this, I find them very
useful. But I agree that there needs to be a better way to assign label
names.

FWIW, I chose a lowercase letter for each function and an uppercase
letter for each label within that function. The camel case (followed by
the colon) created a pair that was unique for grepping.

Petr, in case you missed it, this comment language came from my
discussion[0] with AndreaP.

> I was thinking about renaming labels. E.g.
>
> 	dataring_desc_init()
> 	{
> 		/* di1 */
> 		WRITE_ONCE(desc->begin_lpos, 1);
> 		/* di2 */
> 		WRITE_ONCE(desc->next_lpos, 1);
> 	}
>
> Where di stands for descriptor init.
>
> 	dataring_push()
> 	{
> 		/* dp1 */
> 		ret = get_new_lpos(dr, size, &begin_lpos, &next_lpos);
> 		...
> 		/* dp2 */
> 		smp_mb();
> 		...
> 	}
>
> Where dp stands for descriptor push. For dataring we can add a 'dr'
> prefix, to avoid confusion with desc barriers, which have 'd' prefix.
> And so on. Dunno.

Yeah, I spent a lot of time going in circles on this one.

I hope that we can agree that the labels are important. And that a
formal documentation of the barriers is also important. Yes, they are a
lot of work, but I find it makes it a lot easier to go back to the code
after I've been away for a while. Even now, as I go through your
feedback on code that I wrote over a month ago, I find the formal
comments critical to quickly understand _exactly_ why the memory
barriers exist.

Perhaps we should choose labels that are more clear, like:

    dataring_push:A
    dataring_push:B

Then we would see comments like:

    Memory barrier involvement:

    If _dataring_pop:B reads from dataring_datablock_setid:A, then
    _dataring_pop:C reads from dataring_push:G.

    If _dataring_pop:B reads from dataring_datablock_setid:A, then
    _dataring_pop:D reads from dataring_push:H.

    If _dataring_pop:B reads from dataring_datablock_setid:A, then
    _dataring_pop:E reads from dataring_push:E.

    Note that if _dataring_pop:B reads from dataring_datablock_setid:A, then
    _dataring_pop:C cannot read from dataring_push:C->dataring_desc_init:A.

    Note that if _dataring_pop:B reads from dataring_datablock_setid:A, then
    _dataring_pop:D cannot read from dataring_push:C->dataring_desc_init:B.

    Relies on:

    RELEASE from dataring_push:G to dataring_datablock_setid:A
       matching
    ADDRESS DEP. from _dataring_pop:B to _dataring_pop:C

    RELEASE from dataring_push:H to dataring_datablock_setid:A
       matching
    ADDRESS DEP. from _dataring_pop:B to _dataring_pop:D

    RELEASE from dataring_push:E to dataring_datablock_setid:A
       matching
    ACQUIRE from _dataring_pop:B to _dataring_pop:E

But then how should the labels in the code look? Just the letter looks
simple in code, but cannot be grepped.

    dataring_push()
    {
        ...
        /* E */
        ...
    }

The full label can be grepped, but is redundant with the function name.

    dataring_push()
    {
        ...
        /* dataring_push:E */
        ...
    }

Andrea suggested that the documentation should be within the code, which
I think is a good idea. Even if it means we have more comments than
code.

I am open to suggestions.

John Ogness

[0] https://lkml.kernel.org/r/20190630140855.GA6005@andrea