From: Andrea Parri <parri.andrea@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: David Howells <dhowells@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Will Deacon <will@kernel.org>,
"Paul E. McKenney" <paulmck@linux.ibm.com>,
Mark Rutland <mark.rutland@arm.com>,
Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: Do we need to correct barriering in circular-buffers.rst?
Date: Fri, 27 Sep 2019 11:51:07 +0200 [thread overview]
Message-ID: <20190927095107.GA13098@andrea> (raw)
In-Reply-To: <20190923144931.GC2369@hirez.programming.kicks-ass.net>
On Mon, Sep 23, 2019 at 04:49:31PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 19, 2019 at 02:59:06PM +0100, David Howells wrote:
>
> > But I don't agree with this. You're missing half the barriers. There should
> > be *four* barriers. The document mandates only 3 barriers, and uses
> > READ_ONCE() where the fourth should be, i.e.:
> >
> > thread #1 thread #2
> >
> > smp_load_acquire(head)
> > ... read data from queue ..
> > smp_store_release(tail)
> >
> > READ_ONCE(tail)
> > ... add data to queue ..
> > smp_store_release(head)
> >
>
> Notably your READ_ONCE() pseudo code is lacking a conditional;
> kernel/events/ring_buffer.c writes it like so:
>
> * kernel user
> *
> * if (LOAD ->data_tail) { LOAD ->data_head
> * (A) smp_rmb() (C)
> * STORE $data LOAD $data
> * smp_wmb() (B) smp_mb() (D)
> * STORE ->data_head STORE ->data_tail
> * }
> *
> * Where A pairs with D, and B pairs with C.
> *
> * In our case (A) is a control dependency that separates the load of
> * the ->data_tail and the stores of $data. In case ->data_tail
> * indicates there is no room in the buffer to store $data we do not.
To elaborate on this, dependencies are tricky... ;-)
For the record, the LKMM doesn't currently model "order" derived from
control dependencies to a _plain_ access (even if the plain access is
a write): in particular, the following is racy (as far as the current
LKMM is concerned):
C rb
{ }
P0(int *tail, int *data, int *head)
{
if (READ_ONCE(*tail)) {
*data = 1;
smp_wmb();
WRITE_ONCE(*head, 1);
}
}
P1(int *tail, int *data, int *head)
{
int r0;
int r1;
r0 = READ_ONCE(*head);
smp_rmb();
r1 = *data;
smp_mb();
WRITE_ONCE(*tail, 1);
}
Replacing the plain "*data = 1" with "WRITE_ONCE(*data, 1)" (or doing
s/READ_ONCE(*tail)/smp_load_acquire(tail)) suffices to avoid the race.
Maybe I'm short of imagination this morning... but I can't currently
see how the compiler could "break" the above scenario.
I also didn't spend much time thinking about it. memory-barriers.txt
has a section "CONTROL DEPENDENCIES" dedicated to "alerting developers
using control dependencies for ordering". That's quite a long section
(and probably still incomplete); the last paragraph summarizes: ;-)
(*) Compilers do not understand control dependencies. It is therefore
your job to ensure that they do not break your code.
Andrea
> *
> * D needs to be a full barrier since it separates the data READ
> * from the tail WRITE.
> *
> * For B a WMB is sufficient since it separates two WRITEs, and for C
> * an RMB is sufficient since it separates two READs.
>
> Where 'kernel' is the producer and 'user' is the consumer. This was
> written before load-acquire and store-release came about (I _think_),
> and I've so far resisted updating B to store-release because smp_wmb()
> is actually cheaper than store-release on a number of architectures
> (notably ARM).
>
> C ought to be a load-aquire, and D really should be a store-release, but
> I don't think the perf userspace has that (or uses C11).
next prev parent reply other threads:[~2019-09-27 9:51 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-13 13:00 [RFC][PATCH] pipe: Convert ring to head/tail David Howells
2019-09-13 13:06 ` My just-shovel-data-through-for-X-amount-of-time test David Howells
2019-09-15 14:59 ` [RFC][PATCH] pipe: Convert ring to head/tail Will Deacon
2019-09-17 13:51 ` David Howells
2019-09-17 17:07 ` Will Deacon
2019-09-18 15:43 ` Do we need to correct barriering in circular-buffers.rst? David Howells
2019-09-18 16:48 ` Linus Torvalds
2019-09-19 13:59 ` David Howells
2019-09-19 15:59 ` Linus Torvalds
2019-09-23 14:49 ` Peter Zijlstra
2019-09-27 9:51 ` Andrea Parri [this message]
2019-09-27 12:49 ` Peter Zijlstra
2019-09-27 15:57 ` Peter Zijlstra
2019-09-27 20:43 ` Nick Desaulniers
2019-09-27 21:58 ` Nick Desaulniers
2019-09-30 9:33 ` Peter Zijlstra
2019-09-30 11:54 ` Peter Zijlstra
2019-09-30 12:02 ` Peter Zijlstra
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=20190927095107.GA13098@andrea \
--to=parri.andrea@gmail.com \
--cc=dhowells@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=paulmck@linux.ibm.com \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
--cc=will@kernel.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).