[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191209090321.GF88619@google.com>
Date: Mon, 9 Dec 2019 18:03:21 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc: John Ogness <john.ogness@...utronix.de>,
linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Petr Mladek <pmladek@...e.com>,
Steven Rostedt <rostedt@...dmis.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andrea Parri <andrea.parri@...rulasolutions.com>,
Thomas Gleixner <tglx@...utronix.de>,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Brendan Higgins <brendanhiggins@...gle.com>,
kexec@...ts.infradead.org
Subject: Re: [RFC PATCH v5 2/3] printk-rb: new printk ringbuffer
implementation (reader)
On (19/12/09 17:43), Sergey Senozhatsky wrote:
> > +static int desc_read_committed(struct prb_desc_ring *desc_ring, u32 id,
> > + u64 seq, struct prb_desc *desc)
> > +{
> > + enum desc_state d_state;
> > +
> > + d_state = desc_read(desc_ring, id, desc);
> > + if (desc->info.seq != seq)
> > + return -EINVAL;
> > + else if (d_state == desc_reusable)
> > + return -ENOENT;
> > + else if (d_state != desc_committed)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Copy the ringbuffer data from the record with @seq to the provided
> > + * @r buffer. On success, 0 is returned.
> > + *
> > + * See desc_read_committed() for error return values.
> > + */
> > +static int prb_read(struct printk_ringbuffer *rb, u64 seq,
> > + struct printk_record *r)
> > +{
> > + struct prb_desc_ring *desc_ring = &rb->desc_ring;
> > + struct prb_desc *rdesc = to_desc(desc_ring, seq);
> > + atomic_t *state_var = &rdesc->state_var;
> > + struct prb_desc desc;
> > + int err;
> > + u32 id;
> > +
> > + /* Get a reliable local copy of the descriptor and check validity. */
> > + id = DESC_ID(atomic_read(state_var));
> > + err = desc_read_committed(desc_ring, id, seq, &desc);
> > + if (err)
> > + return err;
> > +
> > + /* If requested, copy meta data. */
> > + if (r->info)
> > + memcpy(r->info, &desc.info, sizeof(*(r->info)));
>
> I wonder if those WARN_ON-s will trigger false positive sometimes.
>
> A theoretical case.
>
> What if reader gets preempted/interrupted in the middle of
> desc_read_committed()->desc_read()->memcpy(). The context which interrupts
> the reader recycles the descriptor and pushes new data. Suppose that
> reader was interrupted right after it copied ->info.seq and ->info.text_len.
> So the first desc_read_committed() will pass - we have matching ->seq
> and committed state. copy_data(), however, most likely, will generate
> WARNs. The final desc_read_committed() will notice that local copy
> of desc was in non-consistent state and everything is fine, but we have
> WARNs in the log buffer now.
Hmm. No, that won't happen. We should get desc_miss first, and then -EINVAL.
-ss
Powered by blists - more mailing lists