[PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Pranith Kumar]

The nocb callbacks generated before the nocb kthreads are spawned are
enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
Parallelize and economize NOCB kthread wakeups") introduced nocb leader kthreads
which checked the nocb_leader_wake flag to see if there were any such pending
callbacks. A case was reported in which newly spawned leader kthreads were not
processing the pending callbacks as this flag was not set, which led to a boot
hang.

The following commit ensures that the newly spawned nocb kthreads process the
pending callbacks by allowing the kthreads to run immediately after spawning
instead of waiting. This is done by inverting the logic of nocb_leader_wake
tests to nocb_leader_sleep which allows us to use the default initialization of
this flag to 0 to let the kthreads run.

Reported-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Link: http://www.spinics.net/lists/kernel/msg1802899.html
---
 kernel/rcu/tree.h        |  2 +-
 kernel/rcu/tree_plugin.h | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 83a7d1c..ffedcb9 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -358,7 +358,7 @@ struct rcu_data {
 	struct rcu_head **nocb_gp_tail;
 	long nocb_gp_count;
 	long nocb_gp_count_lazy;
-	bool nocb_leader_wake;		/* Is the nocb leader thread awake? */
+	bool nocb_leader_sleep;		/* Is the nocb leader thread asleep? */
 	struct rcu_data *nocb_next_follower;
 					/* Next follower in wakeup chain. */
 
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 5780f6d..28d6763 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -2041,9 +2041,9 @@ static void wake_nocb_leader(struct rcu_data *rdp, bool force)
 
 	if (!ACCESS_ONCE(rdp_leader->nocb_kthread))
 		return;
-	if (!ACCESS_ONCE(rdp_leader->nocb_leader_wake) || force) {
+	if (ACCESS_ONCE(rdp_leader->nocb_leader_sleep) || force) {
 		/* Prior smp_mb__after_atomic() orders against prior enqueue. */
-		ACCESS_ONCE(rdp_leader->nocb_leader_wake) = true;
+		ACCESS_ONCE(rdp_leader->nocb_leader_sleep) = false;
 		wake_up(&rdp_leader->nocb_wq);
 	}
 }
@@ -2071,7 +2071,7 @@ static void __call_rcu_nocb_enqueue(struct rcu_data *rdp,
 	ACCESS_ONCE(*old_rhpp) = rhp;
 	atomic_long_add(rhcount, &rdp->nocb_q_count);
 	atomic_long_add(rhcount_lazy, &rdp->nocb_q_count_lazy);
-	smp_mb__after_atomic(); /* Store *old_rhpp before _wake test. */
+	smp_mb__after_atomic(); /* Store *old_rhpp before _sleep test. */
 
 	/* If we are not being polled and there is a kthread, awaken it ... */
 	t = ACCESS_ONCE(rdp->nocb_kthread);
@@ -2239,7 +2239,7 @@ wait_again:
 	if (!rcu_nocb_poll) {
 		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, "Sleep");
 		wait_event_interruptible(my_rdp->nocb_wq,
-					 ACCESS_ONCE(my_rdp->nocb_leader_wake));
+				!ACCESS_ONCE(my_rdp->nocb_leader_sleep));
 		/* Memory barrier handled by smp_mb() calls below and repoll. */
 	} else if (firsttime) {
 		firsttime = false; /* Don't drown trace log with "Poll"! */
@@ -2278,12 +2278,12 @@ wait_again:
 		schedule_timeout_interruptible(1);
 
 		/* Rescan in case we were a victim of memory ordering. */
-		my_rdp->nocb_leader_wake = false;
-		smp_mb();  /* Ensure _wake false before scan. */
+		my_rdp->nocb_leader_sleep = true;
+		smp_mb();  /* Ensure _sleep true before scan. */
 		for (rdp = my_rdp; rdp; rdp = rdp->nocb_next_follower)
 			if (ACCESS_ONCE(rdp->nocb_head)) {
 				/* Found CB, so short-circuit next wait. */
-				my_rdp->nocb_leader_wake = true;
+				my_rdp->nocb_leader_sleep = false;
 				break;
 			}
 		goto wait_again;
@@ -2293,17 +2293,17 @@ wait_again:
 	rcu_nocb_wait_gp(my_rdp);
 
 	/*
-	 * We left ->nocb_leader_wake set to reduce cache thrashing.
-	 * We clear it now, but recheck for new callbacks while
+	 * We left ->nocb_leader_sleep unset to reduce cache thrashing.
+	 * We set it now, but recheck for new callbacks while
 	 * traversing our follower list.
 	 */
-	my_rdp->nocb_leader_wake = false;
-	smp_mb(); /* Ensure _wake false before scan of ->nocb_head. */
+	my_rdp->nocb_leader_sleep = true;
+	smp_mb(); /* Ensure _sleep true before scan of ->nocb_head. */
 
 	/* Each pass through the following loop wakes a follower, if needed. */
 	for (rdp = my_rdp; rdp; rdp = rdp->nocb_next_follower) {
 		if (ACCESS_ONCE(rdp->nocb_head))
-			my_rdp->nocb_leader_wake = true; /* No need to wait. */
+			my_rdp->nocb_leader_sleep = false;/* No need to sleep.*/
 		if (!rdp->nocb_gp_head)
 			continue; /* No CBs, so no need to wake follower. */
 
-- 
1.9.1

Re: [PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Paul E. McKenney]

On Wed, Aug 27, 2014 at 04:43:40PM -0400, Pranith Kumar wrote:
> The nocb callbacks generated before the nocb kthreads are spawned are
> enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
> Parallelize and economize NOCB kthread wakeups") introduced nocb leader kthreads
> which checked the nocb_leader_wake flag to see if there were any such pending
> callbacks. A case was reported in which newly spawned leader kthreads were not
> processing the pending callbacks as this flag was not set, which led to a boot
> hang.
> 
> The following commit ensures that the newly spawned nocb kthreads process the
> pending callbacks by allowing the kthreads to run immediately after spawning
> instead of waiting. This is done by inverting the logic of nocb_leader_wake
> tests to nocb_leader_sleep which allows us to use the default initialization of
> this flag to 0 to let the kthreads run.
> 
> Reported-by: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> Link: http://www.spinics.net/lists/kernel/msg1802899.html

Thank you, Pranith, queued.  I have also backported to v3.17-rc2,
and am testing both.  A sneak preview of the backport is shown below,
please let me know if you see any problems with it.  (The reason for
the backport is to submit to 3.17 as a fix for a regression.)

							Thanx, Paul

------------------------------------------------------------------------

rcu: Make nocb leader kthreads process pending callbacks after spawning

The nocb callbacks generated before the nocb kthreads are spawned are
enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
Parallelize and economize NOCB kthread wakeups") introduced nocb leader kthreads
which checked the nocb_leader_wake flag to see if there were any such pending
callbacks. A case was reported in which newly spawned leader kthreads were not
processing the pending callbacks as this flag was not set, which led to a boot
hang.

The following commit ensures that the newly spawned nocb kthreads process the
pending callbacks by allowing the kthreads to run immediately after spawning
instead of waiting. This is done by inverting the logic of nocb_leader_wake
tests to nocb_leader_sleep which allows us to use the default initialization of
this flag to 0 to let the kthreads run.

Reported-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Link: http://www.spinics.net/lists/kernel/msg1802899.html
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 71e64c718f75..6a86eb7bac45 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -358,7 +358,7 @@ struct rcu_data {
 	struct rcu_head **nocb_gp_tail;
 	long nocb_gp_count;
 	long nocb_gp_count_lazy;
-	bool nocb_leader_wake;		/* Is the nocb leader thread awake? */
+	bool nocb_leader_sleep;		/* Is the nocb leader thread asleep? */
 	struct rcu_data *nocb_next_follower;
 					/* Next follower in wakeup chain. */
 
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 00dc411e9676..a7997e272564 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -2074,9 +2074,9 @@ static void wake_nocb_leader(struct rcu_data *rdp, bool force)
 
 	if (!ACCESS_ONCE(rdp_leader->nocb_kthread))
 		return;
-	if (!ACCESS_ONCE(rdp_leader->nocb_leader_wake) || force) {
+	if (ACCESS_ONCE(rdp_leader->nocb_leader_sleep) || force) {
 		/* Prior xchg orders against prior callback enqueue. */
-		ACCESS_ONCE(rdp_leader->nocb_leader_wake) = true;
+		ACCESS_ONCE(rdp_leader->nocb_leader_sleep) = false;
 		wake_up(&rdp_leader->nocb_wq);
 	}
 }
@@ -2253,7 +2253,7 @@ wait_again:
 	if (!rcu_nocb_poll) {
 		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, "Sleep");
 		wait_event_interruptible(my_rdp->nocb_wq,
-					 ACCESS_ONCE(my_rdp->nocb_leader_wake));
+				!ACCESS_ONCE(my_rdp->nocb_leader_sleep));
 		/* Memory barrier handled by smp_mb() calls below and repoll. */
 	} else if (firsttime) {
 		firsttime = false; /* Don't drown trace log with "Poll"! */
@@ -2292,12 +2292,12 @@ wait_again:
 		schedule_timeout_interruptible(1);
 
 		/* Rescan in case we were a victim of memory ordering. */
-		my_rdp->nocb_leader_wake = false;
-		smp_mb();  /* Ensure _wake false before scan. */
+		my_rdp->nocb_leader_sleep = true;
+		smp_mb();  /* Ensure _sleep true before scan. */
 		for (rdp = my_rdp; rdp; rdp = rdp->nocb_next_follower)
 			if (ACCESS_ONCE(rdp->nocb_head)) {
 				/* Found CB, so short-circuit next wait. */
-				my_rdp->nocb_leader_wake = true;
+				my_rdp->nocb_leader_sleep = false;
 				break;
 			}
 		goto wait_again;
@@ -2307,17 +2307,17 @@ wait_again:
 	rcu_nocb_wait_gp(my_rdp);
 
 	/*
-	 * We left ->nocb_leader_wake set to reduce cache thrashing.
-	 * We clear it now, but recheck for new callbacks while
+	 * We left ->nocb_leader_sleep unset to reduce cache thrashing.
+	 * We set it now, but recheck for new callbacks while
 	 * traversing our follower list.
 	 */
-	my_rdp->nocb_leader_wake = false;
-	smp_mb(); /* Ensure _wake false before scan of ->nocb_head. */
+	my_rdp->nocb_leader_sleep = true;
+	smp_mb(); /* Ensure _sleep true before scan of ->nocb_head. */
 
 	/* Each pass through the following loop wakes a follower, if needed. */
 	for (rdp = my_rdp; rdp; rdp = rdp->nocb_next_follower) {
 		if (ACCESS_ONCE(rdp->nocb_head))
-			my_rdp->nocb_leader_wake = true; /* No need to wait. */
+			my_rdp->nocb_leader_sleep = false;/* No need to sleep.*/
 		if (!rdp->nocb_gp_head)
 			continue; /* No CBs, so no need to wake follower. */
 

Re: [PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Pranith Kumar]

On Wed, Aug 27, 2014 at 5:47 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> Thank you, Pranith, queued.  I have also backported to v3.17-rc2,
> and am testing both.  A sneak preview of the backport is shown below,
> please let me know if you see any problems with it.  (The reason for
> the backport is to submit to 3.17 as a fix for a regression.)
>

Looks good to me!

--
Pranith

Re: [PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Paul E. McKenney]

On Wed, Aug 27, 2014 at 07:08:37PM -0400, Pranith Kumar wrote:
> On Wed, Aug 27, 2014 at 5:47 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > Thank you, Pranith, queued.  I have also backported to v3.17-rc2,
> > and am testing both.  A sneak preview of the backport is shown below,
> > please let me know if you see any problems with it.  (The reason for
> > the backport is to submit to 3.17 as a fix for a regression.)
> 
> Looks good to me!

And both versions pass short rcutorture tests.

Amit, could you please try running your test with the backported
version of the patch?  One more time?

							Thanx, Paul

Re: [PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Amit Shah]

On (Wed) 27 Aug 2014 [16:43:40], Pranith Kumar wrote:
> The nocb callbacks generated before the nocb kthreads are spawned are
> enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
> Parallelize and economize NOCB kthread wakeups") introduced nocb leader kthreads
> which checked the nocb_leader_wake flag to see if there were any such pending
> callbacks. A case was reported in which newly spawned leader kthreads were not
> processing the pending callbacks as this flag was not set, which led to a boot
> hang.
> 
> The following commit ensures that the newly spawned nocb kthreads process the
> pending callbacks by allowing the kthreads to run immediately after spawning
> instead of waiting. This is done by inverting the logic of nocb_leader_wake
> tests to nocb_leader_sleep which allows us to use the default initialization of
> this flag to 0 to let the kthreads run.
> 
> Reported-by: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> Link: http://www.spinics.net/lists/kernel/msg1802899.html
> ---
>  kernel/rcu/tree.h        |  2 +-
>  kernel/rcu/tree_plugin.h | 24 ++++++++++++------------
>  2 files changed, 13 insertions(+), 13 deletions(-)

I'd have split this into two patches: one for the variable rename and
one for fixing the bug.

However, the backport Paul posted does work fine for me on master, so
you can add my

Tested-by: Amit Shah <amit.shah@redhat.com>

Thanks,

		Amit

Re: [PATCH] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Paul E. McKenney]

On Thu, Aug 28, 2014 at 11:16:49AM +0530, Amit Shah wrote:
> On (Wed) 27 Aug 2014 [16:43:40], Pranith Kumar wrote:
> > The nocb callbacks generated before the nocb kthreads are spawned are
> > enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
> > Parallelize and economize NOCB kthread wakeups") introduced nocb leader kthreads
> > which checked the nocb_leader_wake flag to see if there were any such pending
> > callbacks. A case was reported in which newly spawned leader kthreads were not
> > processing the pending callbacks as this flag was not set, which led to a boot
> > hang.
> > 
> > The following commit ensures that the newly spawned nocb kthreads process the
> > pending callbacks by allowing the kthreads to run immediately after spawning
> > instead of waiting. This is done by inverting the logic of nocb_leader_wake
> > tests to nocb_leader_sleep which allows us to use the default initialization of
> > this flag to 0 to let the kthreads run.
> > 
> > Reported-by: Amit Shah <amit.shah@redhat.com>
> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> > Link: http://www.spinics.net/lists/kernel/msg1802899.html
> > ---
> >  kernel/rcu/tree.h        |  2 +-
> >  kernel/rcu/tree_plugin.h | 24 ++++++++++++------------
> >  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> I'd have split this into two patches: one for the variable rename and
> one for fixing the bug.
> 
> However, the backport Paul posted does work fine for me on master, so
> you can add my
> 
> Tested-by: Amit Shah <amit.shah@redhat.com>

Thank you again, Amit, both for finding this problem and for your
testing efforts!

							Thanx, Paul