Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On Tue 2020-03-03 16:42:07, John Ogness wrote:
> On 2020-03-03, Petr Mladek wrote:
> >> diff --git a/kernel/printk/printk_ringbuffer.c
> >> b/kernel/printk/printk_ringbuffer.c
> >> new file mode 100644
> >> index ..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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On Mon 2020-03-02 14:43:41, John Ogness wrote:
> On 2020-03-02, Petr Mladek wrote:
> diff --git a/kernel/printk/printk_ringbuffer.c
> b/kernel/printk/printk_ringbuffer.c
> new file mode 100644
> index ..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)
> +{
> >
> > static enum desc_state
> > desc_read_by_seq(struct prb_desc_ring *desc_ring,
> > u64 seq, struct prb_desc *desc)
> > {
> > struct prb_desc *rdesc = to_desc(desc_ring, seq);
> > atomic_long_t *state_var = &rdesc->state_var;
> > id = DESC_ID(atomic_long_read(state_var));
>
> I think it is error-prone to re-read @state_var here. It is lockless
> shared data. desc_read_committed() is called twice in prb_read() and it
> is expected that both calls are using the same @id.
>
> > enum desc_state d_state;
> >
> > d_state = desc_read(desc_ring, id, desc);
> > if (d_state == desc_miss ||
> > d_state == desc_reserved ||
> > desc->info.seq != seq)
> > return -EINVAL;
> >
> > if (d_state == desc_reusable)
> > return -ENOENT;
>
> I can use this refactoring.
>
> >
> > if (d_state != desc_committed)
> > return -EINVAL;
>
> I suppose you meant to remove this check and leave in the @blk_lpos
> check instead. If we're trying to minimize lines of code, the @blk_lpos
> check could be combined with the "== desc_reusable" check as well.
I am an idiot. I missed that the check "d_state != desc_committed"
will return -EINVAL also when desc_miss or desc_reserved.
I was too concentrated by the fact that desc->info.seq was checked
first even though it might contain garbage.
Also it did not help me much the note about blk_lpos. I did not
see how it was related to this code.
To sum up. The original code worked fine. But I would prefer my variant
that has more lines but it is somehow cleaner.
Best Regards,
Petr
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On 2020-03-03, Petr Mladek wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c
>> b/kernel/printk/printk_ringbuffer.c
>> new file mode 100644
>> index ..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)
>> +{
>>>
>>> OK, what about having desc_read_by_seq() instead?
>>
>> Well, it isn't actually "reading by seq". @seq is there for
>> additional verification. Yes, prb_read() is deriving @id from
>> @seq. But it only does this once and uses that value for both calls.
>
> I do not want to nitpick about words. If I get it properly,
> the "id" is not important here. Any "id" is fine as long as
> "seq" matches. Reading "id" once is just an optimization.
Your statement is incorrect. We are not nitpicking about words. I am
trying to clarify what you are misunderstanding.
@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 do not resist on the change. It was just an idea how to
> avoid confusion. I was confused more than once. But I might
> be the only one. The more strightforward code looked more
> important to me than the optimization.
I am sorry for the confusion. In preparation for v2 I have changed the
function description to:
/*
* Get a copy of a specified descriptor and verify that the record is
* committed and has the sequence number @seq. @seq is checked because
* of possible ABA issues with @id on 32-bit systems. 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.
*/
This is using the same language as the description of desc_read() so
that is it is hopefully clear that desc_read_committed() is an extended
version of desc_read().
>>> Also there is a bug in current desc_read_commited().
>>> desc->info.seq might contain a garbage when d_state is desc_miss
>>> or desc_reserved.
>>
>> It is not a bug. In both of those cases, -EINVAL is the correct return
>> value.
>
> No, it is a bug. If info is not read and contains garbage then the
> following check may pass by chance:
>
> if (desc->info.seq != seq)
> return -EINVAL;
>
> Then the function would return 0 even when desc_read() returned
> desc_miss or desc_reserved.
0 cannot be returned. The state is checked. Please let us stop this
bug/non-bug discussion. It is distracting us from clarifying this
function and refactoring it to simplify understanding.
>>> I would change it to:
>>>
>>> static enum desc_state
>>> desc_read_by_seq(struct prb_desc_ring *desc_ring,
>>> u64 seq, struct prb_desc *desc)
>>> {
>>> struct prb_desc *rdesc = to_desc(desc_ring, seq);
>>> atomic_long_t *state_var = &rdesc->state_var;
>>> id = DESC_ID(atomic_long_read(state_var));
>>
>> I think it is error-prone to re-read @state_var here. It is lockless
>> shared data. desc_read_committed() is called twice in prb_read() and
>> it is expected that both calls are using the same @id.
>
> It is not error prone. If "id" changes then "seq" will not match.
@id is set during prb_reserve(). @seq (being mere meta-data) is set
_afterwards_. Your proposed multiple-deriving of @id from @seq would
work because the _state checks_ would catch it, not because @seq would
necessarily change.
But that logic is backwards. @seq is not what is important here. It is
only meta-data. On 64-bit systems the @seq checks could be safely
removed.
You may want to refer back to your private email [0] from last November
where you asked me to move this code out of prb_read() and into a helper
function. That may clarify what we are talking about (although I hope
the new function description is clear enough).
John Ogness
[0] private: 20191122122724.n6wlummg3ap56...@pathway.suse.cz
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On Mon 2020-03-02 14:43:41, John Ogness wrote:
> On 2020-03-02, Petr Mladek wrote:
> diff --git a/kernel/printk/printk_ringbuffer.c
> b/kernel/printk/printk_ringbuffer.c
> new file mode 100644
> index ..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)
> +{
> >
> > OK, what about having desc_read_by_seq() instead?
>
> Well, it isn't actually "reading by seq". @seq is there for additional
> verification. Yes, prb_read() is deriving @id from @seq. But it only
> does this once and uses that value for both calls.
I do not want to nitpick about words. If I get it properly,
the "id" is not important here. Any "id" is fine as long as
"seq" matches. Reading "id" once is just an optimization.
I do not resist on the change. It was just an idea how to
avoid confusion. I was confused more than once. But I might
be the only one. The more strightforward code looked more
important to me than the optimization.
> > Also there is a bug in current desc_read_commited().
> > desc->info.seq might contain a garbage when d_state is desc_miss
> > or desc_reserved.
>
> It is not a bug. In both of those cases, -EINVAL is the correct return
> value.
No, it is a bug. If info is not read and contains garbage then the
following check may pass by chance:
if (desc->info.seq != seq)
return -EINVAL;
Then the function would return 0 even when desc_read() returned
desc_miss or desc_reserved.
> > I would change it to:
> >
> > static enum desc_state
> > desc_read_by_seq(struct prb_desc_ring *desc_ring,
> > u64 seq, struct prb_desc *desc)
> > {
> > struct prb_desc *rdesc = to_desc(desc_ring, seq);
> > atomic_long_t *state_var = &rdesc->state_var;
> > id = DESC_ID(atomic_long_read(state_var));
>
> I think it is error-prone to re-read @state_var here. It is lockless
> shared data. desc_read_committed() is called twice in prb_read() and it
> is expected that both calls are using the same @id.
It is not error prone. If "id" changes then "seq" will not match.
> > enum desc_state d_state;
> >
> > d_state = desc_read(desc_ring, id, desc);
> > if (d_state == desc_miss ||
> > d_state == desc_reserved ||
> > desc->info.seq != seq)
> > return -EINVAL;
> >
> > if (d_state == desc_reusable)
> > return -ENOENT;
>
> I can use this refactoring.
Yes please, "else" is not needed.
> >
> > if (d_state != desc_committed)
> > return -EINVAL;
>
> I suppose you meant to remove this check and leave in the @blk_lpos
> check instead.
Good catch, this check is superfluous.
> If we're trying to minimize lines of code, the @blk_lpos
> check could be combined with the "== desc_reusable" check as well.
Minimizing the lines of code was not my primary goal. I was just
confused by the function name. Also the fact that "seq" was the
important thing was well hidden.
Best Regards,
Petr
PS: I dived into the barriers and got lost. I hope that I will
be able to send something sensible in the end ;-)
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On 2020-03-02, Petr Mladek wrote:
diff --git a/kernel/printk/printk_ringbuffer.c
b/kernel/printk/printk_ringbuffer.c
new file mode 100644
index ..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)
+{
>
> OK, what about having desc_read_by_seq() instead?
Well, it isn't actually "reading by seq". @seq is there for additional
verification. Yes, prb_read() is deriving @id from @seq. But it only
does this once and uses that value for both calls.
> Also there is a bug in current desc_read_commited().
> desc->info.seq might contain a garbage when d_state is desc_miss
> or desc_reserved.
It is not a bug. In both of those cases, -EINVAL is the correct return
value.
> I would change it to:
>
> static enum desc_state
> desc_read_by_seq(struct prb_desc_ring *desc_ring,
>u64 seq, struct prb_desc *desc)
> {
> struct prb_desc *rdesc = to_desc(desc_ring, seq);
> atomic_long_t *state_var = &rdesc->state_var;
> id = DESC_ID(atomic_long_read(state_var));
I think it is error-prone to re-read @state_var here. It is lockless
shared data. desc_read_committed() is called twice in prb_read() and it
is expected that both calls are using the same @id.
> enum desc_state d_state;
>
> d_state = desc_read(desc_ring, id, desc);
> if (d_state == desc_miss ||
> d_state == desc_reserved ||
> desc->info.seq != seq)
> return -EINVAL;
>
> if (d_state == desc_reusable)
> return -ENOENT;
I can use this refactoring.
>
> if (d_state != desc_committed)
> return -EINVAL;
I suppose you meant to remove this check and leave in the @blk_lpos
check instead. If we're trying to minimize lines of code, the @blk_lpos
check could be combined with the "== desc_reusable" check as well.
>
> return 0;
> }
Thanks.
John Ogness
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On Mon 2020-03-02 11:38:42, John Ogness wrote:
> On 2020-02-21, Petr Mladek wrote:
> >> diff --git a/kernel/printk/printk_ringbuffer.c
> >> b/kernel/printk/printk_ringbuffer.c
> >> new file mode 100644
> >> index ..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)
> >> +{
> >
> > I was few times confused whether this function reads the descriptor
> > a safe way or not.
> >
> > Please, rename it to make it clear that does only a check.
> > For example, check_state_commited().
>
> This function _does_ read. It is a helper function of prb_read() to
> _read_ the descriptor. It is an extended version of desc_read() that
> also performs various checks that the descriptor is committed.
I see.
> I will update the function description to be more similar to desc_read()
> so that it is obvious that it is "getting a copy of a specified
> descriptor".
OK, what about having desc_read_by_seq() instead?
Also there is a bug in current desc_read_commited().
desc->info.seq might contain a garbage when d_state is desc_miss
or desc_reserved.
I would change it to:
static enum desc_state
desc_read_by_seq(struct prb_desc_ring *desc_ring,
u64 seq, struct prb_desc *desc)
{
struct prb_desc *rdesc = to_desc(desc_ring, seq);
atomic_long_t *state_var = &rdesc->state_var;
id = DESC_ID(atomic_long_read(state_var));
enum desc_state d_state;
d_state = desc_read(desc_ring, id, desc);
if (d_state == desc_miss ||
d_state == desc_reserved ||
desc->info.seq != seq)
return -EINVAL;
if (d_state == desc_reusable)
return -ENOENT;
if (d_state != desc_committed)
return -EINVAL;
return 0;
}
Best Regards,
Petr
PS: I am going to dive into the barriers again to answer the last
letter about them.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On Mon, 2020-03-02 at 11:38 +0100, John Ogness wrote:
> On 2020-02-21, Petr Mladek wrote:
> > > diff --git a/kernel/printk/printk_ringbuffer.c
> > > b/kernel/printk/printk_ringbuffer.c
[]
> > > +static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
> > > +unsigned long begin_lpos)
> > > +{
> > > + char *data = &data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
> > > +
> > > + return (struct prb_data_block *)data;
> >
> > Nit: Please, use "blk" instead of "data". I was slightly confused
> > because "data" is also one member of struct prb_data_block.
>
> OK.
trivia:
Perhaps use void * instead of char * and a direct return
and avoid the naming altogether.
static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
unsigned long begin_lpos)
{
return (void *)&data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
}
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer
On 2020-02-21, Petr Mladek wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c
>> b/kernel/printk/printk_ringbuffer.c
>> new file mode 100644
>> index ..796257f226ee
>> --- /dev/null
>> +++ b/kernel/printk/printk_ringbuffer.c
>> +static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
>> + unsigned long begin_lpos)
>> +{
>> +char *data = &data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
>> +
>> +return (struct prb_data_block *)data;
>
> Nit: Please, use "blk" instead of "data". I was slightly confused
> because "data" is also one member of struct prb_data_block.
OK.
>> +/* The possible responses of a descriptor state-query. */
>> +enum desc_state {
>> +desc_miss, /* ID mismatch */
>> +desc_reserved, /* reserved, but still in use by writer */
>> +desc_committed, /* committed, writer is done */
>> +desc_reusable, /* free, not used by any writer */
>
> s/not used/not yet used/
OK.
>> +EXPORT_SYMBOL(prb_reserve);
>
> Please, do not export symbols if there are no plans to actually
> use them from modules. It will be easier to rework the code
> in the future. Nobody would need to worry about external
> users.
>
> Please, do so everywhere in the patchset.
You are correct.
The reason I exported them is that I could run my test module. But since
the test module will not be part of the kernel source, I'll just hack
the exports in when doing my testing.
>> +static char *get_data(struct prb_data_ring *data_ring,
>> + struct prb_data_blk_lpos *blk_lpos,
>> + unsigned long *data_size)
>> +{
>> +struct prb_data_block *db;
>> +
>> +/* Data-less data block description. */
>> +if (blk_lpos->begin == INVALID_LPOS &&
>> +blk_lpos->next == INVALID_LPOS) {
>> +return NULL;
>
> Nit: There is no need for "else" after return. checkpatch.pl usually
> complains about it ;-)
OK.
>> +/*
>> + * 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)
>> +{
>
> I was few times confused whether this function reads the descriptor
> a safe way or not.
>
> Please, rename it to make it clear that does only a check.
> For example, check_state_commited().
This function _does_ read. It is a helper function of prb_read() to
_read_ the descriptor. It is an extended version of desc_read() that
also performs various checks that the descriptor is committed.
I will update the function description to be more similar to desc_read()
so that it is obvious that it is "getting a copy of a specified
descriptor".
John Ogness
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
misc nits Re: [PATCH 1/2] printk: add lockless buffer
Hi,
there are few more small things that catched my eyes during review.
They are from the nits deparment.
On Tue 2020-01-28 17:25:47, John Ogness wrote:
> Introduce a multi-reader multi-writer lockless ringbuffer for storing
> the kernel log messages. Readers and writers may use their API from
> any context (including scheduler and NMI). This ringbuffer will make
> it possible to decouple printk() callers from any context, locking,
> or console constraints. It also makes it possible for readers to have
> full access to the ringbuffer contents at any time and context (for
> example from any panic situation).
>
> diff --git a/kernel/printk/printk_ringbuffer.c
> b/kernel/printk/printk_ringbuffer.c
> new file mode 100644
> index ..796257f226ee
> --- /dev/null
> +++ b/kernel/printk/printk_ringbuffer.c
> +/**
> + * DOC: printk_ringbuffer overview
I really like the overview.
> +/* A data block: maps to the raw data within the data ring. */
> +struct prb_data_block {
> + unsigned long id;
> + chardata[0];
> +};
> +
> +
> +static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
> +unsigned long begin_lpos)
> +{
> + char *data = &data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
> +
> + return (struct prb_data_block *)data;
Nit: Please, use "blk" instead of "data". I was slightly confused
because "data" is also one member of struct prb_data_block.
> +/* The possible responses of a descriptor state-query. */
> +enum desc_state {
> + desc_miss, /* ID mismatch */
> + desc_reserved, /* reserved, but still in use by writer */
> + desc_committed, /* committed, writer is done */
> + desc_reusable, /* free, not used by any writer */
s/not used/not yet used/
> +};
[...]
> +EXPORT_SYMBOL(prb_reserve);
Please, do not export symbols if there are no plans to actually
use them from modules. It will be easier to rework the code
in the future. Nobody would need to worry about external
users.
Please, do so everywhere in the patchset.
> +/*
> + * Given @blk_lpos, return a pointer to the raw data from the data block
> + * and calculate the size of the data part. A NULL pointer is returned
> + * if @blk_lpos specifies values that could never be legal.
> + *
> + * This function (used by readers) performs strict validation on the lpos
> + * values to possibly detect bugs in the writer code. A WARN_ON_ONCE() is
> + * triggered if an internal error is detected.
> + */
> +static char *get_data(struct prb_data_ring *data_ring,
> + struct prb_data_blk_lpos *blk_lpos,
> + unsigned long *data_size)
> +{
> + struct prb_data_block *db;
> +
> + /* Data-less data block description. */
> + if (blk_lpos->begin == INVALID_LPOS &&
> + blk_lpos->next == INVALID_LPOS) {
> + return NULL;
Nit: There is no need for "else" after return. checkpatch.pl usually
complains about it ;-)
> +
> + /* Regular data block: @begin less than @next and in same wrap. */
> + } else if (DATA_WRAPS(data_ring, blk_lpos->begin) ==
> +DATA_WRAPS(data_ring, blk_lpos->next) &&
> +blk_lpos->begin < blk_lpos->next) {
> + db = to_block(data_ring, blk_lpos->begin);
> + *data_size = blk_lpos->next - blk_lpos->begin;
> +
> + /* Wrapping data block: @begin is one wrap behind @next. */
> + } else if (DATA_WRAPS(data_ring,
> + blk_lpos->begin + DATA_SIZE(data_ring)) ==
> +DATA_WRAPS(data_ring, blk_lpos->next)) {
> + db = to_block(data_ring, 0);
> + *data_size = DATA_INDEX(data_ring, blk_lpos->next);
> +
> + /* Illegal block description. */
> + } else {
> + WARN_ON_ONCE(1);
> + return NULL;
> + }
> +
> + /* A valid data block will always be aligned to the ID size. */
> + if (WARN_ON_ONCE(blk_lpos->begin !=
> + ALIGN(blk_lpos->begin, sizeof(db->id))) ||
> + WARN_ON_ONCE(blk_lpos->next !=
> + ALIGN(blk_lpos->next, sizeof(db->id {
> + return NULL;
> + }
> +
> + /* A valid data block will always have at least an ID. */
> + if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> + return NULL;
> +
> + /* Subtract descriptor ID space from size to reflect data size. */
> + *data_size -= sizeof(db->id);
> +
> + return &db->data[0];
> +}
> +
> +/*
> + * 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
