On Mon 2020-03-02 14:43:41, John Ogness wrote:
> On 2020-03-02, 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)
> >>>> +{
> >
> > 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
