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

[Petr Mladek]

On Fri 2019-08-23 14:54:45, Sergey Senozhatsky wrote:
> On (08/21/19 07:46), John Ogness wrote:
> [..]
> > 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.
> [..]
> > > 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.
> 
> Yeah. I like those tagsi/labels, and appreciate your efforts.
> 
> Speaking about it in general, not necessarily related to printk patch set.
> With or without labels/tags we still have to grep. But grep-ing is much
> easier when we have labels/tags. Otherwise it's sometimes hard to understand
> what to grep for - _acquire, _relaxed, smp barrier, write_once, or
> anything else.

Grepping is not needed when function names are used in the comment
and cscope might be used. Each function should be short
and easy enough so that any nested label can be found by eyes.

A custom script is an alternative but it would be better to use
existing tools.

In each case, two letter labels would get redundant sooner or later
when the semantic gets used widely. And I hope that we use a semantic
that is going to be used widely.

> > 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.
> [..]
> >     RELEASE from dataring_push:E to dataring_datablock_setid:A
> >        matching
> >     ACQUIRE from _dataring_pop:B to _dataring_pop:E
> 
> I thought about it. That's very informative, albeit pretty hard to maintain.
> The same applies to drA or prA and any other context dependent prefix.

The maintenance is my concern as well. The labels should be primary
for an automatized consistency checker. They make sense only when
they are in sync with the code.

Best Regards,
Petr