lkml.org 
[lkml]   [2020]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: misc nits Re: [PATCH 1/2] printk: add lockless buffer
Date
On 2020-03-02, 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)
>>>> +{
>
> 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

\
 
 \ /
  Last update: 2020-03-02 14:44    [W:0.113 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site