lkml.org 
[lkml]   [2020]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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 <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)
> >> +{
> >
> > 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.

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