On Tue 2020-03-03 16:42:07, John Ogness wrote:
> On 2020-03-03, Petr Mladek <pmladek@xxxxxxxx> 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
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
