All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: Ingo Molnar <mingo@redhat.com>
Cc: Nicholas Piggin <npiggin@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org
Subject: [PATCH] swait: add missing barrier to swake_up
Date: Fri,  1 Sep 2017 16:14:50 +1000	[thread overview]
Message-ID: <20170901061450.1450-1-npiggin@gmail.com> (raw)

swake_up and swake_up_all test the swaitqueue outside the lock,
but they are missing the barrier that would ensure visibility
of a previous store that sets the wakeup condition with the
load that tests the swaitqueue. This could lead to a lost wakeup
if there is memory reordering. Fix this as prescribed by the
waitqueue_active comments.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
--
I noticed this when chasing down that rcu hang bug (which
turned out to not be anything of the sort). I might be missing
something here and it's safe somehow, but if so then it should
have a comment where it diverges from normal waitqueues.

It looks like there's a few callers which are also testing
swait_active before swake_up without a barrier which look wrong,
so I must be missing something but I'm not sure what.

Thanks,
Nick
---
 kernel/sched/swait.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
index 3d5610dcce11..9056278001d9 100644
--- a/kernel/sched/swait.c
+++ b/kernel/sched/swait.c
@@ -33,6 +33,11 @@ void swake_up(struct swait_queue_head *q)
 {
 	unsigned long flags;
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;
 
@@ -51,6 +56,11 @@ void swake_up_all(struct swait_queue_head *q)
 	struct swait_queue *curr;
 	LIST_HEAD(tmp);
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;
 
-- 
2.13.3

             reply	other threads:[~2017-09-01  6:15 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-01  6:14 Nicholas Piggin [this message]
2017-09-01  9:23 ` [PATCH] swait: add missing barrier to swake_up Andrea Parri
2017-09-01  9:55   ` Nicholas Piggin
2017-09-01 14:34     ` Paul E. McKenney
2017-09-01 11:16 ` 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=20170901061450.1450-1-npiggin@gmail.com \
    --to=npiggin@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.