From: John Ogness <john.ogness@linutronix.de>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: linux-kernel@vger.kernel.org,
Peter Zijlstra <peterz@infradead.org>,
Petr Mladek <pmladek@suse.com>,
Steven Rostedt <rostedt@goodmis.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Andrea Parri <andrea.parri@amarulasolutions.com>,
Thomas Gleixner <tglx@linutronix.de>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Brendan Higgins <brendanhiggins@google.com>,
kexec@lists.infradead.org
Subject: Re: [RFC PATCH v5 2/3] printk-rb: new printk ringbuffer implementation (reader)
Date: Mon, 09 Dec 2019 10:09:16 +0100 [thread overview]
Message-ID: <87r21dzwzn.fsf@linutronix.de> (raw)
In-Reply-To: <20191209084300.GD88619@google.com> (Sergey Senozhatsky's message of "Mon, 9 Dec 2019 17:43:00 +0900")
On 2019-12-09, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>> +/* Given @blk_lpos, copy an expected @len of data into the provided buffer. */
>> +static bool copy_data(struct prb_data_ring *data_ring,
>> + struct prb_data_blk_lpos *blk_lpos, u16 len, char *buf,
>> + unsigned int buf_size)
>> +{
>> + unsigned long data_size;
>> + char *data;
>> +
>> + /* Caller might not want the data. */
>> + if (!buf || !buf_size)
>> + return true;
>> +
>> + data = get_data(data_ring, blk_lpos, &data_size);
>> + if (!data)
>> + return false;
>> +
>> + /* Actual cannot be less than expected. */
>> + if (WARN_ON_ONCE(data_size < len))
>> + return false;
>> +
>> + data_size = min_t(u16, buf_size, len);
>> +
>> + if (!WARN_ON_ONCE(!data_size))
>> + memcpy(&buf[0], data, data_size);
>> + return true;
>> +}
>> +
>> +/*
>> + * Read the record @id and verify that it is committed and has the sequence
>> + * number @seq.
>> + *
>> + * Error return values:
>> + * -EINVAL: The 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, 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.
Be aware that desc_read_committed() is filling a copy of the descriptor
in the local variable @desc. If desc_read_committed() succeeded, that
local copy _must_ be consistent. If the WARNs trigger, either
desc_read_committed() or the writer code is broken.
John Ogness
next prev parent reply other threads:[~2019-12-09 9:09 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-28 1:52 [RFC PATCH v5 0/3] printk: new ringbuffer implementation John Ogness
2019-11-28 1:52 ` [RFC PATCH v5 1/3] printk-rb: new printk ringbuffer implementation (writer) John Ogness
2019-12-02 15:48 ` Petr Mladek
2019-12-02 15:59 ` Petr Mladek
2019-12-02 16:37 ` John Ogness
2019-12-03 1:17 ` Sergey Senozhatsky
2019-12-03 14:18 ` Steven Rostedt
2019-12-05 12:01 ` John Ogness
2019-12-03 8:54 ` Petr Mladek
2019-12-03 14:13 ` John Ogness
2019-12-03 14:36 ` Petr Mladek
2019-12-09 9:19 ` Sergey Senozhatsky
2019-12-09 7:42 ` Sergey Senozhatsky
2019-12-09 9:00 ` John Ogness
2019-12-09 9:27 ` Sergey Senozhatsky
2019-12-09 9:34 ` John Ogness
2019-12-21 14:22 ` Andrea Parri
2019-12-23 16:01 ` John Ogness
2020-01-03 10:24 ` Petr Mladek
2020-01-04 14:33 ` Andrea Parri
2019-11-28 1:52 ` [RFC PATCH v5 2/3] printk-rb: new printk ringbuffer implementation (reader) John Ogness
2019-12-03 12:06 ` Petr Mladek
2019-12-03 13:46 ` John Ogness
2019-12-04 12:54 ` Petr Mladek
2019-12-04 13:28 ` John Ogness
2019-12-09 8:43 ` Sergey Senozhatsky
2019-12-09 9:03 ` Sergey Senozhatsky
2019-12-09 9:09 ` John Ogness [this message]
2019-11-28 1:52 ` [RFC PATCH v5 3/3] printk-rb: add test module John Ogness
2019-12-09 8:44 ` Sergey Senozhatsky
2019-12-05 13:46 ` [RFC PATCH v5 0/3] printk: new ringbuffer implementation Prarit Bhargava
2019-12-05 14:05 ` John Ogness
2019-12-06 14:16 ` Prarit Bhargava
2020-01-27 12:20 ` Eugeniu Rosca
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r21dzwzn.fsf@linutronix.de \
--to=john.ogness@linutronix.de \
--cc=andrea.parri@amarulasolutions.com \
--cc=brendanhiggins@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).