linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Petr Mladek <pmladek@suse.com>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.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, 9 Dec 2019 17:43:00 +0900	[thread overview]
Message-ID: <20191209084300.GD88619@google.com> (raw)
In-Reply-To: <20191128015235.12940-3-john.ogness@linutronix.de>

On (19/11/28 02:58), John Ogness 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.

	-ss

  parent reply	other threads:[~2019-12-09  8:43 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 [this message]
2019-12-09  9:03     ` Sergey Senozhatsky
2019-12-09  9:09     ` John Ogness
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=20191209084300.GD88619@google.com \
    --to=sergey.senozhatsky.work@gmail.com \
    --cc=andrea.parri@amarulasolutions.com \
    --cc=brendanhiggins@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=john.ogness@linutronix.de \
    --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@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).