From: Peter Zijlstra <peterz@infradead.org>
To: Waiman Long <longman@redhat.com>
Cc: Jan Stancek <jstancek@redhat.com>,
linux-kernel@vger.kernel.org, dbueso@suse.de, will@kernel.org,
mingo@redhat.com
Subject: Re: [PATCH] locking/rwsem: use read_acquire in read_slowpath exit when queue is empty
Date: Tue, 16 Jul 2019 20:58:08 +0200 [thread overview]
Message-ID: <20190716185807.GJ3402@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <4ef66a01-7937-1eb7-c58b-0992a0142c92@redhat.com>
On Tue, Jul 16, 2019 at 12:53:14PM -0400, Waiman Long wrote:
> On 7/16/19 12:04 PM, Jan Stancek wrote:
> > Suspected problem here is that last *_acquire on down_read() side
> > happens before write side issues *_release:
> > 1. writer: has the lock
> > 2. reader: down_read() issues *read_acquire on entry
> > 3. writer: mm->vmacache_seqnum++; downgrades lock (*fetch_add_release)
> > 4. reader: __rwsem_down_read_failed_common() finds it can take lock and returns
> > 5. reader: observes stale mm->vmacache_seqnum
> >
> > I can reproduce the problem by running LTP mtest06 in a loop and building
> > kernel (-j $NCPUS) in parallel. It does reproduce since v4.20 up to v5.2
> > on arm64 HPE Apollo 70 (224 CPUs, 256GB RAM, 2 nodes). It triggers reliably
> > within ~hour. Patched kernel ran fine for 5+ hours with clean dmesg.
> > Tests were done against v5.2, since commit cf69482d62d9 ("locking/rwsem:
> > Enable readers spinning on writer") makes it much harder to reproduce.
> > Fixes: 4b486b535c33 ("locking/rwsem: Exit read lock slowpath if queue empty & no writer")
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > Cc: Waiman Long <longman@redhat.com>
> > Cc: Davidlohr Bueso <dbueso@suse.de>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > ---
> > kernel/locking/rwsem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> > index 37524a47f002..757b198d7a5b 100644
> > --- a/kernel/locking/rwsem.c
> > +++ b/kernel/locking/rwsem.c
> > @@ -1030,7 +1030,7 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
> > * exit the slowpath and return immediately as its
> > * RWSEM_READER_BIAS has already been set in the count.
> > */
> > - if (adjustment && !(atomic_long_read(&sem->count) &
> > + if (adjustment && !(atomic_long_read_acquire(&sem->count) &
> > (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
> > raw_spin_unlock_irq(&sem->wait_lock);
> > rwsem_set_reader_owned(sem);
>
> The chance of taking this path is not that high. So instead of
> increasing the cost of the test by adding an acquire barrier, how about
> just adding smp_mb__after_spinlock() before spin_unlock_irq(). This
> should have the same effect of making sure that no stale data will be
> used in the read-lock critical section.
That's actually more expensive on something like ARM64 I expect.
The far cheaper alternative is smp_acquire__after_ctrl_dep(), however in
general Will seems to prefer using load-acquire over separate barriers,
and for x86 it doesn't matter anyway. For PowerPC these two are a wash,
both end up with LWSYNC (over SYNC for your alternative).
next prev parent reply other threads:[~2019-07-16 18:58 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-16 16:04 [PATCH] locking/rwsem: use read_acquire in read_slowpath exit when queue is empty Jan Stancek
2019-07-16 16:53 ` Waiman Long
2019-07-16 18:34 ` Jan Stancek
2019-07-16 18:58 ` Peter Zijlstra [this message]
2019-07-16 19:09 ` Waiman Long
2019-07-17 12:02 ` [PATCH v2] locking/rwsem: add acquire barrier to " Jan Stancek
2019-07-17 13:13 ` Will Deacon
2019-07-17 14:19 ` Waiman Long
2019-07-17 19:22 ` Jan Stancek
2019-07-17 19:39 ` Waiman Long
2019-07-18 8:51 ` [PATCH v3] " Jan Stancek
2019-07-25 16:00 ` [tip:locking/core] locking/rwsem: Add missing ACQUIRE " tip-bot for Jan Stancek
2019-07-18 9:26 ` [PATCH v2] locking/rwsem: add acquire barrier " Will Deacon
2019-07-18 10:50 ` Jan Stancek
2019-07-18 11:04 ` Peter Zijlstra
2019-07-18 11:09 ` Peter Zijlstra
2019-07-18 11:36 ` Jan Stancek
2019-07-18 12:12 ` Paul E. McKenney
2019-07-18 10:58 ` Peter Zijlstra
2019-07-18 11:45 ` Will Deacon
2019-07-18 12:23 ` Peter Zijlstra
2019-07-17 15:33 ` Waiman Long
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=20190716185807.GJ3402@hirez.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=dbueso@suse.de \
--cc=jstancek@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=mingo@redhat.com \
--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).