[PATCH 0/5] srcu fixes

[PATCH 0/5] srcu fixes

[Frederic Weisbecker]

Hi,

This contains a fix for "SRCU: kworker hung in synchronize_srcu":

	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com

And a few cleanups.

Passed 50 hours of SRCU-P and SRCU-N.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	srcu/fixes

HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0

Thanks,
	Frederic
---

Frederic Weisbecker (5):
      srcu: Fix callbacks acceleration mishandling
      srcu: Only accelerate on enqueue time
      srcu: Remove superfluous callbacks advancing from srcu_start_gp()
      srcu: No need to advance/accelerate if no callback enqueued
      srcu: Explain why callbacks invocations can't run concurrently


 kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 16 deletions(-)

[PATCH 1/5] srcu: Fix callbacks acceleration mishandling

[Frederic Weisbecker]

SRCU callbacks acceleration might fail if the preceding callbacks
advance also fails. This can happen when the following steps are met:

1) The RCU_WAIT_TAIL segment has callbacks (say for gp_num 8) and the
   RCU_NEXT_READY_TAIL also has callbacks (say for gp_num 12).

2) The grace period for RCU_WAIT_TAIL is observed as started but not yet
   completed so rcu_seq_current() returns 4 + SRCU_STATE_SCAN1 = 5.

3) This value is passed to rcu_segcblist_advance() which can't move
   any segment forward and fails.

4) srcu_gp_start_if_needed() still proceeds with callback acceleration.
   But then the call to rcu_seq_snap() observes the grace period for the
   RCU_WAIT_TAIL segment (gp_num 8) as completed and the subsequent one
   for the RCU_NEXT_READY_TAIL segment as started
   (ie: 8 + SRCU_STATE_SCAN1 = 9) so it returns a snapshot of the
   next grace period, which is 16.

5) The value of 16 is passed to rcu_segcblist_accelerate() but the
   freshly enqueued callback in RCU_NEXT_TAIL can't move to
   RCU_NEXT_READY_TAIL which already has callbacks for a previous grace
   period (gp_num = 12). So acceleration fails.

6) Note in all these steps, srcu_invoke_callbacks() hadn't had a chance
   to run srcu_invoke_callbacks().

Then some very bad outcome may happen if the following happens:

7) Some other CPU races and starts the grace period number 16 before the
   CPU handling previous steps had a chance. Therefore srcu_gp_start()
   isn't called on the latter sdp to fix the acceleration leak from
   previous steps with a new pair of call to advance/accelerate.

8) The grace period 16 completes and srcu_invoke_callbacks() is finally
   called. All the callbacks from previous grace periods (8 and 12) are
   correctly advanced and executed but callbacks in RCU_NEXT_READY_TAIL
   still remain. Then rcu_segcblist_accelerate() is called with a
   snaphot of 20.

9) Since nothing started the grace period number 20, callbacks stay
   unhandled.

This has been reported in real load:

	[3144162.608392] INFO: task kworker/136:12:252684 blocked for more
	than 122 seconds.
	[3144162.615986]       Tainted: G           O  K   5.4.203-1-tlinux4-0011.1 #1
	[3144162.623053] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
	disables this message.
	[3144162.631162] kworker/136:12  D    0 252684      2 0x90004000
	[3144162.631189] Workqueue: kvm-irqfd-cleanup irqfd_shutdown [kvm]
	[3144162.631192] Call Trace:
	[3144162.631202]  __schedule+0x2ee/0x660
	[3144162.631206]  schedule+0x33/0xa0
	[3144162.631209]  schedule_timeout+0x1c4/0x340
	[3144162.631214]  ? update_load_avg+0x82/0x660
	[3144162.631217]  ? raw_spin_rq_lock_nested+0x1f/0x30
	[3144162.631218]  wait_for_completion+0x119/0x180
	[3144162.631220]  ? wake_up_q+0x80/0x80
	[3144162.631224]  __synchronize_srcu.part.19+0x81/0xb0
	[3144162.631226]  ? __bpf_trace_rcu_utilization+0x10/0x10
	[3144162.631227]  synchronize_srcu+0x5f/0xc0
	[3144162.631236]  irqfd_shutdown+0x3c/0xb0 [kvm]
	[3144162.631239]  ? __schedule+0x2f6/0x660
	[3144162.631243]  process_one_work+0x19a/0x3a0
	[3144162.631244]  worker_thread+0x37/0x3a0
	[3144162.631247]  kthread+0x117/0x140
	[3144162.631247]  ? process_one_work+0x3a0/0x3a0
	[3144162.631248]  ? __kthread_cancel_work+0x40/0x40
	[3144162.631250]  ret_from_fork+0x1f/0x30

Fix this with taking the snapshot for acceleration _before_ the read
of the current grace period number.

The only side effect of this solution is that callbacks advancing happen
then _after_ the full barrier in rcu_seq_snap(). This is not a problem
because that barrier only cares about:

1) Ordering accesses of the update side before call_srcu() so they don't
   bleed.
2) See all the accesses prior to the grace period of the current gp_num

The only things callbacks advancing need to be ordered against are
carried by snp locking.

Reported-by: Yong He <zhuangel570@gmail.com>
Co-developed-by: Yong He <zhuangel570@gmail.com>
Co-developed-by: Joel Fernandes <joel@joelfernandes.org>
Co-developed-by: Neeraj upadhyay <neeraj.iitr10@gmail.com>
Link: http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
Fixes: da915ad5cf25 ("srcu: Parallelize callback handling")
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/srcutree.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 5602042856b1..9fab9ac36996 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1244,10 +1244,37 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	spin_lock_irqsave_sdp_contention(sdp, &flags);
 	if (rhp)
 		rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
-	rcu_segcblist_advance(&sdp->srcu_cblist,
-			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+	/*
+	 * The snapshot for acceleration must be taken _before_ the read of the
+	 * current gp sequence used for advancing, otherwise advancing may fail
+	 * and acceleration may then fail too.
+	 *
+	 * This could happen if:
+	 *
+	 *  1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the
+	 *     RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8).
+	 *
+	 *  2) The grace period for RCU_WAIT_TAIL is seen as started but not
+	 *     completed so rcu_seq_current() returns X + SRCU_STATE_SCAN1.
+	 *
+	 *  3) This value is passed to rcu_segcblist_advance() which can't move
+	 *     any segment forward and fails.
+	 *
+	 *  4) srcu_gp_start_if_needed() still proceeds with callback acceleration.
+	 *     But then the call to rcu_seq_snap() observes the grace period for the
+	 *     RCU_WAIT_TAIL segment as completed and the subsequent one for the
+	 *     RCU_NEXT_READY_TAIL segment as started (ie: X + 4 + SRCU_STATE_SCAN1)
+	 *     so it returns a snapshot of the next grace period, which is X + 12.
+	 *
+	 *  5) The value of X + 12 is passed to rcu_segcblist_accelerate() but the
+	 *     freshly enqueued callback in RCU_NEXT_TAIL can't move to
+	 *     RCU_NEXT_READY_TAIL which already has callbacks for a previous grace
+	 *     period (gp_num = X + 8). So acceleration fails.
+	 */
 	s = rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq);
-	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist, s);
+	rcu_segcblist_advance(&sdp->srcu_cblist,
+			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+	WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s) && rhp);
 	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
 		sdp->srcu_gp_seq_needed = s;
 		needgp = true;
-- 
2.41.0

[PATCH 2/5] srcu: Only accelerate on enqueue time

[Frederic Weisbecker]

Acceleration in SRCU happens on enqueue time for each new callback. This
operation is expected not to fail and therefore any similar attempt
from other places shouldn't find any remaining callbacks to accelerate.

Moreover accelerations performed beyond enqueue time are error prone
because rcu_seq_snap() then may return the snapshot for a new grace
period that is not going to be started.

Remove these dangerous and needless accelerations and introduce instead
assertions reporting leaking unaccelerated callbacks beyond enqueue
time.

Co-developed-by: Yong He <zhuangel570@gmail.com>
Co-developed-by: Joel Fernandes <joel@joelfernandes.org>
Co-developed-by: Neeraj upadhyay <neeraj.iitr10@gmail.com>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/srcutree.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 9fab9ac36996..560e99ec5333 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -784,8 +784,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
 	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
-	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
-				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
+	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
 	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
 	WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
 	WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
@@ -1721,6 +1720,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
 	ssp = sdp->ssp;
 	rcu_cblist_init(&ready_cbs);
 	spin_lock_irq_rcu_node(sdp);
+	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
 	if (sdp->srcu_cblist_invoking ||
@@ -1750,8 +1750,6 @@ static void srcu_invoke_callbacks(struct work_struct *work)
 	 */
 	spin_lock_irq_rcu_node(sdp);
 	rcu_segcblist_add_len(&sdp->srcu_cblist, -len);
-	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
-				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
 	sdp->srcu_cblist_invoking = false;
 	more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
 	spin_unlock_irq_rcu_node(sdp);
-- 
2.41.0

[PATCH 3/5] srcu: Remove superfluous callbacks advancing from srcu_start_gp()

[Frederic Weisbecker]

Callbacks advancing on SRCU must be performed on two specific places:

1) On enqueue time in order to make room for the acceleration of the
   new callback.

2) On invocation time in order to move the callbacks ready to invoke.

Any other callback advancing callsite is needless. Remove the remaining
one in srcu_gp_start().

Co-developed-by: Yong He <zhuangel570@gmail.com>
Co-developed-by: Joel Fernandes <joel@joelfernandes.org>
Co-developed-by: Neeraj upadhyay <neeraj.iitr10@gmail.com>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/srcutree.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 560e99ec5333..e9356a103626 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -772,20 +772,10 @@ EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
  */
 static void srcu_gp_start(struct srcu_struct *ssp)
 {
-	struct srcu_data *sdp;
 	int state;
 
-	if (smp_load_acquire(&ssp->srcu_sup->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
-		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
-	else
-		sdp = this_cpu_ptr(ssp->sda);
 	lockdep_assert_held(&ACCESS_PRIVATE(ssp->srcu_sup, lock));
 	WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_sup->srcu_gp_seq, ssp->srcu_sup->srcu_gp_seq_needed));
-	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
-	rcu_segcblist_advance(&sdp->srcu_cblist,
-			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
-	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
-	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
 	WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
 	WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
 	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
-- 
2.41.0

[PATCH 4/5] srcu: No need to advance/accelerate if no callback enqueued

[Frederic Weisbecker]

While in grace period start, there is nothing to accelerate and
therefore no need to advance the callbacks either if no callback is
to be enqueued.

Spare these needless operations in this case.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/srcutree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index e9356a103626..2bfc8ed1eed2 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1261,9 +1261,11 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	 *     period (gp_num = X + 8). So acceleration fails.
 	 */
 	s = rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq);
-	rcu_segcblist_advance(&sdp->srcu_cblist,
-			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
-	WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s) && rhp);
+	if (rhp) {
+		rcu_segcblist_advance(&sdp->srcu_cblist,
+				      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+		WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s));
+	}
 	if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
 		sdp->srcu_gp_seq_needed = s;
 		needgp = true;
-- 
2.41.0

[PATCH 5/5] srcu: Explain why callbacks invocations can't run concurrently

[Frederic Weisbecker]

If an SRCU barrier is queued while callbacks are running and a new
callbacks invocator for the same sdp were to run concurrently, the
RCU barrier might execute too early. As this requirement is non-obvious,
make sure to keep a record.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/rcu/srcutree.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 2bfc8ed1eed2..0351a4e83529 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
 	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+	/*
+	 * Although this function is theoretically re-entrant, concurrent
+	 * callbacks invocation is disallowed to avoid executing an SRCU barrier
+	 * too early.
+	 */
 	if (sdp->srcu_cblist_invoking ||
 	    !rcu_segcblist_ready_cbs(&sdp->srcu_cblist)) {
 		spin_unlock_irq_rcu_node(sdp);
@@ -1745,6 +1750,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
 	sdp->srcu_cblist_invoking = false;
 	more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
 	spin_unlock_irq_rcu_node(sdp);
+	/* An SRCU barrier or callbacks from previous nesting work pending */
 	if (more)
 		srcu_schedule_cbs_sdp(sdp, 0);
 }
-- 
2.41.0

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> Hi,
> 
> This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> 
> 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> 
> And a few cleanups.
> 
> Passed 50 hours of SRCU-P and SRCU-N.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	srcu/fixes
> 
> HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> 
> Thanks,
> 	Frederic

Very good, and a big "Thank You!!!" to all of you!

I queued this series for testing purposes, and have started a bunch of
SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
SRCU-N on another system, but with both scenarios resized to 40 CPU each.

While that is in flight, a few questions:

o	Please check the Co-developed-by rules.  Last I knew, it was
	necessary to have a Signed-off-by after each Co-developed-by.

o	Is it possible to get a Tested-by from the original reporter?
	Or is this not reproducible?

o	Is it possible to convince rcutorture to find this sort of
	bug?  Seems like it should be, but easy to say...

o	Frederic, would you like to include this in your upcoming
	pull request?  Or does it need more time?

						Thanx, Paul

> ---
> 
> Frederic Weisbecker (5):
>       srcu: Fix callbacks acceleration mishandling
>       srcu: Only accelerate on enqueue time
>       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
>       srcu: No need to advance/accelerate if no callback enqueued
>       srcu: Explain why callbacks invocations can't run concurrently
> 
> 
>  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > Hi,
> > 
> > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > 
> > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > 
> > And a few cleanups.
> > 
> > Passed 50 hours of SRCU-P and SRCU-N.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > 	srcu/fixes
> > 
> > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > 
> > Thanks,
> > 	Frederic
> 
> Very good, and a big "Thank You!!!" to all of you!
> 
> I queued this series for testing purposes, and have started a bunch of
> SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> 
> While that is in flight, a few questions:
> 
> o	Please check the Co-developed-by rules.  Last I knew, it was
> 	necessary to have a Signed-off-by after each Co-developed-by.
> 
> o	Is it possible to get a Tested-by from the original reporter?
> 	Or is this not reproducible?
> 
> o	Is it possible to convince rcutorture to find this sort of
> 	bug?  Seems like it should be, but easy to say...

And one other thing...

o	What other bugs like this one are hiding elsewhere
	in RCU?

> o	Frederic, would you like to include this in your upcoming
> 	pull request?  Or does it need more time?

						Thanx, Paul

> > ---
> > 
> > Frederic Weisbecker (5):
> >       srcu: Fix callbacks acceleration mishandling
> >       srcu: Only accelerate on enqueue time
> >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> >       srcu: No need to advance/accelerate if no callback enqueued
> >       srcu: Explain why callbacks invocations can't run concurrently
> > 
> > 
> >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Tue, Oct 03, 2023 at 08:21:42PM -0700, Paul E. McKenney wrote:
> On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > Hi,
> > > 
> > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > > 
> > > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > > 
> > > And a few cleanups.
> > > 
> > > Passed 50 hours of SRCU-P and SRCU-N.
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > 	srcu/fixes
> > > 
> > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > > 
> > > Thanks,
> > > 	Frederic
> > 
> > Very good, and a big "Thank You!!!" to all of you!
> > 
> > I queued this series for testing purposes, and have started a bunch of
> > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > SRCU-N on another system, but with both scenarios resized to 40 CPU each.

The 200*1h of SRCU-N and the 100*1h of SRCU-p passed other than the usual
tick-stop errors.  (Is there a patch for that one?)  The 40-CPU SRCU-N
run was fine, but the 40-CPU SRCU-P run failed due to the fanouts setting
a maximum of 16 CPUs.  So I started a 10-hour 40-CPU SRCU-P and a pair
of 10-hour 16-CPU SRCU-N runs on one system, and 200*10h of SRCU-N and
100*10h of SRCU-P.

I will let you know how it goes.

							Thanx, Paul

> > While that is in flight, a few questions:
> > 
> > o	Please check the Co-developed-by rules.  Last I knew, it was
> > 	necessary to have a Signed-off-by after each Co-developed-by.
> > 
> > o	Is it possible to get a Tested-by from the original reporter?
> > 	Or is this not reproducible?
> > 
> > o	Is it possible to convince rcutorture to find this sort of
> > 	bug?  Seems like it should be, but easy to say...
> 
> And one other thing...
> 
> o	What other bugs like this one are hiding elsewhere
> 	in RCU?
> 
> > o	Frederic, would you like to include this in your upcoming
> > 	pull request?  Or does it need more time?
> 
> 						Thanx, Paul
> 
> > > ---
> > > 
> > > Frederic Weisbecker (5):
> > >       srcu: Fix callbacks acceleration mishandling
> > >       srcu: Only accelerate on enqueue time
> > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > >       srcu: No need to advance/accelerate if no callback enqueued
> > >       srcu: Explain why callbacks invocations can't run concurrently
> > > 
> > > 
> > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > Hi,
> > 
> > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > 
> > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > 
> > And a few cleanups.
> > 
> > Passed 50 hours of SRCU-P and SRCU-N.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > 	srcu/fixes
> > 
> > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > 
> > Thanks,
> > 	Frederic
> 
> Very good, and a big "Thank You!!!" to all of you!
> 
> I queued this series for testing purposes, and have started a bunch of
> SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> 
> While that is in flight, a few questions:
> 
> o	Please check the Co-developed-by rules.  Last I knew, it was
> 	necessary to have a Signed-off-by after each Co-developed-by.

Indeed! I'll try to collect the three of them within a few days. If some
are missing, I'll put a Reported-by instead.

> 
> o	Is it possible to get a Tested-by from the original reporter?
> 	Or is this not reproducible?

It seems that the issue would trigger rarely. But I hope we can get one.

> 
> o	Is it possible to convince rcutorture to find this sort of
> 	bug?  Seems like it should be, but easy to say...

So at least the part where advance/accelerate fail is observed from time
to time. But then we must meet two more rare events:

1) The CPU failing to ACC/ADV must also fail to start the grace period because
  another CPU was faster.

2) The callbacks invocation must not run until that grace period has ended (even
  though we had a previous one completed with callbacks ready).

  Or it can run after all but at least the acceleration part of it has to
  happen after the end of the new grace period.

Perhaps all these conditions can me met more often if we overcommit the number
of vCPU. For example run 10 SRCU-P instances within 3 real CPUs. This could
introduce random breaks within the torture writers...

Just an idea...

> 
> o	Frederic, would you like to include this in your upcoming
> 	pull request?  Or does it need more time?

At least the first patch yes. It should be easily backported and
it should be enough to solve the race. I'll just wait a bit to collect
more tags.

Thanks!

> 
> 						Thanx, Paul
> 
> > ---
> > 
> > Frederic Weisbecker (5):
> >       srcu: Fix callbacks acceleration mishandling
> >       srcu: Only accelerate on enqueue time
> >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> >       srcu: No need to advance/accelerate if no callback enqueued
> >       srcu: Explain why callbacks invocations can't run concurrently
> > 
> > 
> >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

On Tue, Oct 03, 2023 at 08:21:42PM -0700, Paul E. McKenney wrote:
> On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > Hi,
> > > 
> > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > > 
> > > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > > 
> > > And a few cleanups.
> > > 
> > > Passed 50 hours of SRCU-P and SRCU-N.
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > 	srcu/fixes
> > > 
> > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > > 
> > > Thanks,
> > > 	Frederic
> > 
> > Very good, and a big "Thank You!!!" to all of you!
> > 
> > I queued this series for testing purposes, and have started a bunch of
> > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> > 
> > While that is in flight, a few questions:
> > 
> > o	Please check the Co-developed-by rules.  Last I knew, it was
> > 	necessary to have a Signed-off-by after each Co-developed-by.
> > 
> > o	Is it possible to get a Tested-by from the original reporter?
> > 	Or is this not reproducible?
> > 
> > o	Is it possible to convince rcutorture to find this sort of
> > 	bug?  Seems like it should be, but easy to say...
> 
> And one other thing...
> 
> o	What other bugs like this one are hiding elsewhere
> 	in RCU?

Hmm, yesterday I thought RCU would be fine because it has a tick polling
on callbacks anyway. But I'm not so sure, I'll check for real...

Thanks.

> 
> > o	Frederic, would you like to include this in your upcoming
> > 	pull request?  Or does it need more time?
> 
> 						Thanx, Paul
> 
> > > ---
> > > 
> > > Frederic Weisbecker (5):
> > >       srcu: Fix callbacks acceleration mishandling
> > >       srcu: Only accelerate on enqueue time
> > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > >       srcu: No need to advance/accelerate if no callback enqueued
> > >       srcu: Explain why callbacks invocations can't run concurrently
> > > 
> > > 
> > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

On Tue, Oct 03, 2023 at 08:30:45PM -0700, Paul E. McKenney wrote:
> On Tue, Oct 03, 2023 at 08:21:42PM -0700, Paul E. McKenney wrote:
> > On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > > Hi,
> > > > 
> > > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > > > 
> > > > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > > > 
> > > > And a few cleanups.
> > > > 
> > > > Passed 50 hours of SRCU-P and SRCU-N.
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > > 	srcu/fixes
> > > > 
> > > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > > > 
> > > > Thanks,
> > > > 	Frederic
> > > 
> > > Very good, and a big "Thank You!!!" to all of you!
> > > 
> > > I queued this series for testing purposes, and have started a bunch of
> > > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > > SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> 
> The 200*1h of SRCU-N and the 100*1h of SRCU-p passed other than the usual
> tick-stop errors.  (Is there a patch for that one?)  The 40-CPU SRCU-N
> run was fine, but the 40-CPU SRCU-P run failed due to the fanouts setting
> a maximum of 16 CPUs.  So I started a 10-hour 40-CPU SRCU-P and a pair
> of 10-hour 16-CPU SRCU-N runs on one system, and 200*10h of SRCU-N and
> 100*10h of SRCU-P.
> 
> I will let you know how it goes.

Very nice! It might be worth testing the first patch alone as
well if we backport only this one.

Thanks!


> 							Thanx, Paul
> 
> > > While that is in flight, a few questions:
> > > 
> > > o	Please check the Co-developed-by rules.  Last I knew, it was
> > > 	necessary to have a Signed-off-by after each Co-developed-by.
> > > 
> > > o	Is it possible to get a Tested-by from the original reporter?
> > > 	Or is this not reproducible?
> > > 
> > > o	Is it possible to convince rcutorture to find this sort of
> > > 	bug?  Seems like it should be, but easy to say...
> > 
> > And one other thing...
> > 
> > o	What other bugs like this one are hiding elsewhere
> > 	in RCU?
> > 
> > > o	Frederic, would you like to include this in your upcoming
> > > 	pull request?  Or does it need more time?
> > 
> > 						Thanx, Paul
> > 
> > > > ---
> > > > 
> > > > Frederic Weisbecker (5):
> > > >       srcu: Fix callbacks acceleration mishandling
> > > >       srcu: Only accelerate on enqueue time
> > > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > > >       srcu: No need to advance/accelerate if no callback enqueued
> > > >       srcu: Explain why callbacks invocations can't run concurrently
> > > > 
> > > > 
> > > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Wed, Oct 04, 2023 at 11:36:49AM +0200, Frederic Weisbecker wrote:
> On Tue, Oct 03, 2023 at 08:30:45PM -0700, Paul E. McKenney wrote:
> > On Tue, Oct 03, 2023 at 08:21:42PM -0700, Paul E. McKenney wrote:
> > > On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > > > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > > > Hi,
> > > > > 
> > > > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > > > > 
> > > > > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > > > > 
> > > > > And a few cleanups.
> > > > > 
> > > > > Passed 50 hours of SRCU-P and SRCU-N.
> > > > > 
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > > > 	srcu/fixes
> > > > > 
> > > > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > > > > 
> > > > > Thanks,
> > > > > 	Frederic
> > > > 
> > > > Very good, and a big "Thank You!!!" to all of you!
> > > > 
> > > > I queued this series for testing purposes, and have started a bunch of
> > > > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > > > SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> > 
> > The 200*1h of SRCU-N and the 100*1h of SRCU-p passed other than the usual
> > tick-stop errors.  (Is there a patch for that one?)  The 40-CPU SRCU-N
> > run was fine, but the 40-CPU SRCU-P run failed due to the fanouts setting
> > a maximum of 16 CPUs.  So I started a 10-hour 40-CPU SRCU-P and a pair
> > of 10-hour 16-CPU SRCU-N runs on one system, and 200*10h of SRCU-N and
> > 100*10h of SRCU-P.
> > 
> > I will let you know how it goes.
> 
> Very nice! It might be worth testing the first patch alone as
> well if we backport only this one.

The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
completed without failure.  The others had some failures, but I need
to look and see if any were unexpected.  In the meantime, I started a
two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)

							Thanx, Paul

> Thanks!
> 
> 
> > 							Thanx, Paul
> > 
> > > > While that is in flight, a few questions:
> > > > 
> > > > o	Please check the Co-developed-by rules.  Last I knew, it was
> > > > 	necessary to have a Signed-off-by after each Co-developed-by.
> > > > 
> > > > o	Is it possible to get a Tested-by from the original reporter?
> > > > 	Or is this not reproducible?
> > > > 
> > > > o	Is it possible to convince rcutorture to find this sort of
> > > > 	bug?  Seems like it should be, but easy to say...
> > > 
> > > And one other thing...
> > > 
> > > o	What other bugs like this one are hiding elsewhere
> > > 	in RCU?
> > > 
> > > > o	Frederic, would you like to include this in your upcoming
> > > > 	pull request?  Or does it need more time?
> > > 
> > > 						Thanx, Paul
> > > 
> > > > > ---
> > > > > 
> > > > > Frederic Weisbecker (5):
> > > > >       srcu: Fix callbacks acceleration mishandling
> > > > >       srcu: Only accelerate on enqueue time
> > > > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > > > >       srcu: No need to advance/accelerate if no callback enqueued
> > > > >       srcu: Explain why callbacks invocations can't run concurrently
> > > > > 
> > > > > 
> > > > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > > > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Wed, Oct 04, 2023 at 07:06:58AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 04, 2023 at 11:36:49AM +0200, Frederic Weisbecker wrote:
> > On Tue, Oct 03, 2023 at 08:30:45PM -0700, Paul E. McKenney wrote:
> > > On Tue, Oct 03, 2023 at 08:21:42PM -0700, Paul E. McKenney wrote:
> > > > On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > > > > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > > > > > 
> > > > > > 	http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > > > > > 
> > > > > > And a few cleanups.
> > > > > > 
> > > > > > Passed 50 hours of SRCU-P and SRCU-N.
> > > > > > 
> > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > > > > > 	srcu/fixes
> > > > > > 
> > > > > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > > > > > 
> > > > > > Thanks,
> > > > > > 	Frederic
> > > > > 
> > > > > Very good, and a big "Thank You!!!" to all of you!
> > > > > 
> > > > > I queued this series for testing purposes, and have started a bunch of
> > > > > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > > > > SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> > > 
> > > The 200*1h of SRCU-N and the 100*1h of SRCU-p passed other than the usual
> > > tick-stop errors.  (Is there a patch for that one?)  The 40-CPU SRCU-N
> > > run was fine, but the 40-CPU SRCU-P run failed due to the fanouts setting
> > > a maximum of 16 CPUs.  So I started a 10-hour 40-CPU SRCU-P and a pair
> > > of 10-hour 16-CPU SRCU-N runs on one system, and 200*10h of SRCU-N and
> > > 100*10h of SRCU-P.
> > > 
> > > I will let you know how it goes.
> > 
> > Very nice! It might be worth testing the first patch alone as
> > well if we backport only this one.
> 
> The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
> completed without failure.  The others had some failures, but I need
> to look and see if any were unexpected.  In the meantime, I started a
> two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
> just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)

And the two-hour 40-CPU SRCU-P run and a pair of two-hour 16-CPU SRCU-N
runs (on only the first commit) completed without incident.

The other set of overnight full-stack runs had only tick-stop errors,
so I started a two-hour set on the first commit.

So far so good!

							Thanx, Paul

> > Thanks!
> > 
> > 
> > > 							Thanx, Paul
> > > 
> > > > > While that is in flight, a few questions:
> > > > > 
> > > > > o	Please check the Co-developed-by rules.  Last I knew, it was
> > > > > 	necessary to have a Signed-off-by after each Co-developed-by.
> > > > > 
> > > > > o	Is it possible to get a Tested-by from the original reporter?
> > > > > 	Or is this not reproducible?
> > > > > 
> > > > > o	Is it possible to convince rcutorture to find this sort of
> > > > > 	bug?  Seems like it should be, but easy to say...
> > > > 
> > > > And one other thing...
> > > > 
> > > > o	What other bugs like this one are hiding elsewhere
> > > > 	in RCU?
> > > > 
> > > > > o	Frederic, would you like to include this in your upcoming
> > > > > 	pull request?  Or does it need more time?
> > > > 
> > > > 						Thanx, Paul
> > > > 
> > > > > > ---
> > > > > > 
> > > > > > Frederic Weisbecker (5):
> > > > > >       srcu: Fix callbacks acceleration mishandling
> > > > > >       srcu: Only accelerate on enqueue time
> > > > > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > > > > >       srcu: No need to advance/accelerate if no callback enqueued
> > > > > >       srcu: Explain why callbacks invocations can't run concurrently
> > > > > > 
> > > > > > 
> > > > > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > > > > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

Le Wed, Oct 04, 2023 at 09:47:04AM -0700, Paul E. McKenney a écrit :
> > The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
> > completed without failure.  The others had some failures, but I need
> > to look and see if any were unexpected.  In the meantime, I started a
> > two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
> > just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)
> 
> And the two-hour 40-CPU SRCU-P run and a pair of two-hour 16-CPU SRCU-N
> runs (on only the first commit) completed without incident.
> 
> The other set of overnight full-stack runs had only tick-stop errors,
> so I started a two-hour set on the first commit.
> 
> So far so good!

Very nice!

As for the tick-stop error, see the upstream fix:

   1a6a46477494 ("timers: Tag (hr)timer softirq as hotplug safe")

Thanks!

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Wed, Oct 04, 2023 at 11:27:29PM +0200, Frederic Weisbecker wrote:
> Le Wed, Oct 04, 2023 at 09:47:04AM -0700, Paul E. McKenney a écrit :
> > > The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
> > > completed without failure.  The others had some failures, but I need
> > > to look and see if any were unexpected.  In the meantime, I started a
> > > two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
> > > just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)
> > 
> > And the two-hour 40-CPU SRCU-P run and a pair of two-hour 16-CPU SRCU-N
> > runs (on only the first commit) completed without incident.
> > 
> > The other set of overnight full-stack runs had only tick-stop errors,
> > so I started a two-hour set on the first commit.
> > 
> > So far so good!
> 
> Very nice!
> 
> As for the tick-stop error, see the upstream fix:
> 
>    1a6a46477494 ("timers: Tag (hr)timer softirq as hotplug safe")

Got it, thank you!

And the two-hour set of 200*SRCU-N and 100*SRCU-P had only tick-stop
errors.  I am refreshing the test grid, and will run overnight.

Here is hoping!

							Thanx, Paul

Re: [PATCH 0/5] srcu fixes

[Paul E. McKenney]

On Wed, Oct 04, 2023 at 02:54:57PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 04, 2023 at 11:27:29PM +0200, Frederic Weisbecker wrote:
> > Le Wed, Oct 04, 2023 at 09:47:04AM -0700, Paul E. McKenney a écrit :
> > > > The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
> > > > completed without failure.  The others had some failures, but I need
> > > > to look and see if any were unexpected.  In the meantime, I started a
> > > > two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
> > > > just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)
> > > 
> > > And the two-hour 40-CPU SRCU-P run and a pair of two-hour 16-CPU SRCU-N
> > > runs (on only the first commit) completed without incident.
> > > 
> > > The other set of overnight full-stack runs had only tick-stop errors,
> > > so I started a two-hour set on the first commit.
> > > 
> > > So far so good!
> > 
> > Very nice!
> > 
> > As for the tick-stop error, see the upstream fix:
> > 
> >    1a6a46477494 ("timers: Tag (hr)timer softirq as hotplug safe")
> 
> Got it, thank you!
> 
> And the two-hour set of 200*SRCU-N and 100*SRCU-P had only tick-stop
> errors.  I am refreshing the test grid, and will run overnight.

And the ten-hour test passed with only the tick-stop errors, representing
2000 hours of SRCU-N and 1000 hours of SRCU-P.  (I ran the exact same
stack, without the rebased fix you call out above.)

Looking good!

							Thanx, Paul

Re: [PATCH 0/5] srcu fixes

[zhuangel570]

On Wed, Oct 4, 2023 at 5:25 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> On Tue, Oct 03, 2023 at 05:35:31PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 04, 2023 at 01:28:58AM +0200, Frederic Weisbecker wrote:
> > > Hi,
> > >
> > > This contains a fix for "SRCU: kworker hung in synchronize_srcu":
> > >
> > >     http://lore.kernel.org/CANZk6aR+CqZaqmMWrC2eRRPY12qAZnDZLwLnHZbNi=xXMB401g@mail.gmail.com
> > >
> > > And a few cleanups.
> > >
> > > Passed 50 hours of SRCU-P and SRCU-N.
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > >     srcu/fixes
> > >
> > > HEAD: 7ea5adc5673b42ef06e811dca75e43d558cc87e0
> > >
> > > Thanks,
> > >     Frederic
> >
> > Very good, and a big "Thank You!!!" to all of you!
> >
> > I queued this series for testing purposes, and have started a bunch of
> > SRCU-P and SRCU-N tests on one set of systems, and a single SRCU-P and
> > SRCU-N on another system, but with both scenarios resized to 40 CPU each.
> >
> > While that is in flight, a few questions:
> >
> > o     Please check the Co-developed-by rules.  Last I knew, it was
> >       necessary to have a Signed-off-by after each Co-developed-by.
>
> Indeed! I'll try to collect the three of them within a few days. If some
> are missing, I'll put a Reported-by instead.
>
> >
> > o     Is it possible to get a Tested-by from the original reporter?
> >       Or is this not reproducible?
>
> It seems that the issue would trigger rarely. But I hope we can get one.

There is currently no way to reproduce this problem in our environment.
The problem has appeared on 2 machines, and each time it occurred, the
test had been running for more than a month.

BTW, I will run tests with these patches in our environment.

>
> >
> > o     Is it possible to convince rcutorture to find this sort of
> >       bug?  Seems like it should be, but easy to say...
>
> So at least the part where advance/accelerate fail is observed from time
> to time. But then we must meet two more rare events:
>
> 1) The CPU failing to ACC/ADV must also fail to start the grace period because
>   another CPU was faster.
>
> 2) The callbacks invocation must not run until that grace period has ended (even
>   though we had a previous one completed with callbacks ready).
>
>   Or it can run after all but at least the acceleration part of it has to
>   happen after the end of the new grace period.
>
> Perhaps all these conditions can me met more often if we overcommit the number
> of vCPU. For example run 10 SRCU-P instances within 3 real CPUs. This could
> introduce random breaks within the torture writers...
>
> Just an idea...
>
> >
> > o     Frederic, would you like to include this in your upcoming
> >       pull request?  Or does it need more time?
>
> At least the first patch yes. It should be easily backported and
> it should be enough to solve the race. I'll just wait a bit to collect
> more tags.
>
> Thanks!
>
> >
> >                                               Thanx, Paul
> >
> > > ---
> > >
> > > Frederic Weisbecker (5):
> > >       srcu: Fix callbacks acceleration mishandling
> > >       srcu: Only accelerate on enqueue time
> > >       srcu: Remove superfluous callbacks advancing from srcu_start_gp()
> > >       srcu: No need to advance/accelerate if no callback enqueued
> > >       srcu: Explain why callbacks invocations can't run concurrently
> > >
> > >
> > >  kernel/rcu/srcutree.c | 55 ++++++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 39 insertions(+), 16 deletions(-)

Re: [PATCH 2/5] srcu: Only accelerate on enqueue time

[Like Xu]

On 4/10/2023 7:29 am, Frederic Weisbecker wrote:
> Acceleration in SRCU happens on enqueue time for each new callback. This
> operation is expected not to fail and therefore any similar attempt
> from other places shouldn't find any remaining callbacks to accelerate.
> 
> Moreover accelerations performed beyond enqueue time are error prone
> because rcu_seq_snap() then may return the snapshot for a new grace
> period that is not going to be started.
> 
> Remove these dangerous and needless accelerations and introduce instead
> assertions reporting leaking unaccelerated callbacks beyond enqueue
> time.
> 
> Co-developed-by: Yong He <zhuangel570@gmail.com>
> Co-developed-by: Joel Fernandes <joel@joelfernandes.org>
> Co-developed-by: Neeraj upadhyay <neeraj.iitr10@gmail.com>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   kernel/rcu/srcutree.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 9fab9ac36996..560e99ec5333 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -784,8 +784,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
>   	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
>   	rcu_segcblist_advance(&sdp->srcu_cblist,
>   			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> -				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
> +	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
>   	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
>   	WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
>   	WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
> @@ -1721,6 +1720,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   	ssp = sdp->ssp;
>   	rcu_cblist_init(&ready_cbs);
>   	spin_lock_irq_rcu_node(sdp);
> +	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
>   	rcu_segcblist_advance(&sdp->srcu_cblist,
>   			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
>   	if (sdp->srcu_cblist_invoking ||
> @@ -1750,8 +1750,6 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   	 */
>   	spin_lock_irq_rcu_node(sdp);
>   	rcu_segcblist_add_len(&sdp->srcu_cblist, -len);
> -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> -				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));

We did observe such issues at our farm and the same diff was applied,
thus this fix is especially appreciated. Thanks.

Reviewed-by: Like Xu <likexu@tencent.com>

>   	sdp->srcu_cblist_invoking = false;
>   	more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
>   	spin_unlock_irq_rcu_node(sdp);

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

On Thu, Oct 05, 2023 at 09:54:12AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 04, 2023 at 02:54:57PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 04, 2023 at 11:27:29PM +0200, Frederic Weisbecker wrote:
> > > Le Wed, Oct 04, 2023 at 09:47:04AM -0700, Paul E. McKenney a écrit :
> > > > > The 10-hour 40-CPU SRCU-P run and pair of 10-hour 16-CPU SRCU-N runs
> > > > > completed without failure.  The others had some failures, but I need
> > > > > to look and see if any were unexpected.  In the meantime, I started a
> > > > > two-hour 40-CPU SRCU-P run and a pair of one-hour 16-CPU SRCU-N runs on
> > > > > just that first commit.  Also servicing SIGSHOWER and SIGFOOD.  ;-)
> > > > 
> > > > And the two-hour 40-CPU SRCU-P run and a pair of two-hour 16-CPU SRCU-N
> > > > runs (on only the first commit) completed without incident.
> > > > 
> > > > The other set of overnight full-stack runs had only tick-stop errors,
> > > > so I started a two-hour set on the first commit.
> > > > 
> > > > So far so good!
> > > 
> > > Very nice!
> > > 
> > > As for the tick-stop error, see the upstream fix:
> > > 
> > >    1a6a46477494 ("timers: Tag (hr)timer softirq as hotplug safe")
> > 
> > Got it, thank you!
> > 
> > And the two-hour set of 200*SRCU-N and 100*SRCU-P had only tick-stop
> > errors.  I am refreshing the test grid, and will run overnight.
> 
> And the ten-hour test passed with only the tick-stop errors, representing
> 2000 hours of SRCU-N and 1000 hours of SRCU-P.  (I ran the exact same
> stack, without the rebased fix you call out above.)

Thanks a lot!

Re: [PATCH 0/5] srcu fixes

[Frederic Weisbecker]

On Sat, Oct 07, 2023 at 06:24:53PM +0800, zhuangel570 wrote:
> On Wed, Oct 4, 2023 at 5:25 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> There is currently no way to reproduce this problem in our environment.
> The problem has appeared on 2 machines, and each time it occurred, the
> test had been running for more than a month.
> 
> BTW, I will run tests with these patches in our environment.

Ok, let us know if it ever triggers after this series.

Since I added you in the Co-developed-by: tags, I will need to
also add your Signed-off-by: tag.

Is that ok for you?

Thanks.

Re: [PATCH 0/5] srcu fixes

[zhuangel570]

On Tue, Oct 10, 2023 at 7:27 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> On Sat, Oct 07, 2023 at 06:24:53PM +0800, zhuangel570 wrote:
> > On Wed, Oct 4, 2023 at 5:25 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> > There is currently no way to reproduce this problem in our environment.
> > The problem has appeared on 2 machines, and each time it occurred, the
> > test had been running for more than a month.
> >
> > BTW, I will run tests with these patches in our environment.
>
> Ok, let us know if it ever triggers after this series.

Sure, I have ported the patch set to our test environment, now 2 machines
already run test for 3 days, everything looks fine.

The patch set is very elegant and completely eliminates the possibility of
unexpected accelerations in our analysis. I am very confident in fixing our
problem.

>
> Since I added you in the Co-developed-by: tags, I will need to
> also add your Signed-off-by: tag.
>
> Is that ok for you?

Sure. Big thanks!
If possible, would you please change my "Signed-off-by" into:

Signed-off-by: Yong He <alexyonghe@tencent.com>

>
> Thanks.