Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer

[Petr Mladek <pmladek@suse.com>]

On Tue 2020-03-03 16:42:07, John Ogness wrote:
> On 2020-03-03, Petr Mladek <pmladek@suse.com> wrote:
> >>>>>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..796257f226ee
> >>>>>> --- /dev/null
> >>>>>> +++ b/kernel/printk/printk_ringbuffer.c
> >>>>>> +/*
> >>>>>> + * Read the record @id and verify that it is committed and has the sequence
> >>>>>> + * number @seq. On success, 0 is returned.
> >>>>>> + *
> >>>>>> + * Error return values:
> >>>>>> + * -EINVAL: A committed record @seq does not exist.
> >>>>>> + * -ENOENT: The record @seq exists, but its data is not available. This is a
> >>>>>> + *          valid record, so readers should continue with the next seq.
> >>>>>> + */
> >>>>>> +static int desc_read_committed(struct prb_desc_ring *desc_ring,
> >>>>>> +			       unsigned long id, u64 seq,
> >>>>>> +			       struct prb_desc *desc)
> >>>>>> +{
> >>>
> @id _is_ very important because that is how descriptors are
> read. desc_read() takes @id as an argument and it is @id that identifies
> the descriptor. @seq is only meta-data within a descriptor. The only
> reason @seq is even checked is because of possible ABA issues with @id
> on 32-bit systems.

I think that the different view is because I look at this API
from the reader API side. It is called the following way:

prb_read_valid(, seq, )
  _prb_read_valid( , &seq, )
    prb_read( , *seq, )
        # id is read from address defined by seq
	rdesc = dr->descs[seq & MASK];
	id = rdesc->state_var && MASK_ID;

        desc_read_commited( , id, seq, )
	  desc_read( , id, )
	    # desc is the same as rdesc above because
	    # seq & MASK == id & MASK
	    desc = dr->descs[id & MASK];

Note that prb_read_valid() and prb_read() are addressed by seq.

It would be perfectly fine to pass only seq to desc_read_committed()
and read id from inside.

The name desc_read_committed() suggests that the important condition
is that the descriptor is in the committed state. It is not obvious
that seq is important as well.

From my POV, it will be more clear to pass only seq and rename the
function to desc_read_by_seq() or so:

  + seq is enough for addressing
  + function returns true only when the stored seq matches
  + the stored seq is valid only when the state is committed
    or reusable


Please, do not reply to this mail. Either take the idea or keep
the code as is. I could live with it. And it is not important
enough to spend more time on it. I just wanted to explain my view.
But it is obviously just a personal preference.

Best Regards,
Petr