[PATCH 0/5] misc RCU fixes and cleanups

[PATCH 0/5] misc RCU fixes and cleanups

[Joel Fernandes (Google)]

Here are some RCU fixes and cleanups. The main one is the TREE07 stuttering fix
which makes the test pass now. I am continuing to test these but I am sending
it out for review in advance. Thanks.

Joel Fernandes (Google) (5):
  rcutorture: Fix stuttering races and other issues
  srcu: Fix error handling in init_srcu_struct_fields()
  tree/nocb: Adjust RCU_NOCB_WAKE_* macros from weaker to stronger
  tree/nocb: Improve readability of nocb_gp_wait()
  Revert "checkpatch: Error out if deprecated RCU API used"

 kernel/rcu/srcutree.c  |  31 ++++++-----
 kernel/rcu/tree.h      |   4 +-
 kernel/rcu/tree_nocb.h | 113 ++++++++++++++++++++++++-----------------
 kernel/torture.c       |  46 +++++------------
 scripts/checkpatch.pl  |   9 ----
 5 files changed, 98 insertions(+), 105 deletions(-)

-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 1/5] rcutorture: Fix stuttering races and other issues

[Joel Fernandes (Google)]

The stuttering code isn't functioning as expected. Ideally, it should
pause the torture threads for a designated period before resuming. Yet,
it fails to halt the test for the correct duration. Additionally, a race
condition exists, potentially causing the stuttering code to pause for
an extended period if the 'spt' variable is non-zero due to the stutter
orchestration thread's inadequate CPU time.

Moreover, over-stuttering can hinder RCU's progress on TREE07 kernels.
This happens as the stuttering code may run within a softirq due to RCU
callbacks. Consequently, ksoftirqd keeps a CPU busy for several seconds,
thus obstructing RCU's progress. This situation triggers a warning
message in the logs:

[ 2169.481783] rcu_torture_writer: rtort_pipe_count: 9

This warning suggests that an RCU torture object, although invisible to
RCU readers, couldn't make it past the pipe array and be freed -- a
strong indication that there weren't enough grace periods during the
stutter interval.

To address these issues, this patch sets the "stutter end" time to an
absolute point in the future set by the main stutter thread. This is
then used for waiting in stutter_wait(). While the stutter thread still
defines this absolute time, the waiters' waiting logic doesn't rely on
the stutter thread receiving sufficient CPU time to halt the stuttering
as the halting is now self-controlled.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/torture.c | 46 +++++++++++++---------------------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/kernel/torture.c b/kernel/torture.c
index 68dba4ecab5c..63f8f2a7d960 100644
--- a/kernel/torture.c
+++ b/kernel/torture.c
@@ -719,7 +719,7 @@ static void torture_shutdown_cleanup(void)
  * suddenly applied to or removed from the system.
  */
 static struct task_struct *stutter_task;
-static int stutter_pause_test;
+static ktime_t stutter_till_abs_time;
 static int stutter;
 static int stutter_gap;
 
@@ -729,30 +729,17 @@ static int stutter_gap;
  */
 bool stutter_wait(const char *title)
 {
-	unsigned int i = 0;
 	bool ret = false;
-	int spt;
+	ktime_t now_ns, till_ns;
 
 	cond_resched_tasks_rcu_qs();
-	spt = READ_ONCE(stutter_pause_test);
-	for (; spt; spt = READ_ONCE(stutter_pause_test)) {
-		if (!ret && !rt_task(current)) {
-			sched_set_normal(current, MAX_NICE);
-			ret = true;
-		}
-		if (spt == 1) {
-			torture_hrtimeout_jiffies(1, NULL);
-		} else if (spt == 2) {
-			while (READ_ONCE(stutter_pause_test)) {
-				if (!(i++ & 0xffff))
-					torture_hrtimeout_us(10, 0, NULL);
-				cond_resched();
-			}
-		} else {
-			torture_hrtimeout_jiffies(round_jiffies_relative(HZ), NULL);
-		}
-		torture_shutdown_absorb(title);
+	now_ns = ktime_get();
+	till_ns = READ_ONCE(stutter_till_abs_time);
+	if (till_ns && ktime_before(now_ns, till_ns)) {
+		torture_hrtimeout_ns(ktime_sub(till_ns, now_ns), 0, NULL);
+		ret = true;
 	}
+	torture_shutdown_absorb(title);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(stutter_wait);
@@ -763,23 +750,16 @@ EXPORT_SYMBOL_GPL(stutter_wait);
  */
 static int torture_stutter(void *arg)
 {
-	DEFINE_TORTURE_RANDOM(rand);
-	int wtime;
+	ktime_t till_ns;
 
 	VERBOSE_TOROUT_STRING("torture_stutter task started");
 	do {
 		if (!torture_must_stop() && stutter > 1) {
-			wtime = stutter;
-			if (stutter > 2) {
-				WRITE_ONCE(stutter_pause_test, 1);
-				wtime = stutter - 3;
-				torture_hrtimeout_jiffies(wtime, &rand);
-				wtime = 2;
-			}
-			WRITE_ONCE(stutter_pause_test, 2);
-			torture_hrtimeout_jiffies(wtime, NULL);
+			till_ns = ktime_add_ns(ktime_get(),
+					       jiffies_to_nsecs(stutter));
+			WRITE_ONCE(stutter_till_abs_time, till_ns);
+			torture_hrtimeout_jiffies(stutter - 1, NULL);
 		}
-		WRITE_ONCE(stutter_pause_test, 0);
 		if (!torture_must_stop())
 			torture_hrtimeout_jiffies(stutter_gap, NULL);
 		torture_shutdown_absorb("torture_stutter");
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 1/5] rcutorture: Fix stuttering races and other issues

[Paul E. McKenney]

On Tue, Jul 25, 2023 at 11:29:06PM +0000, Joel Fernandes (Google) wrote:
> The stuttering code isn't functioning as expected. Ideally, it should
> pause the torture threads for a designated period before resuming. Yet,
> it fails to halt the test for the correct duration. Additionally, a race
> condition exists, potentially causing the stuttering code to pause for
> an extended period if the 'spt' variable is non-zero due to the stutter
> orchestration thread's inadequate CPU time.
> 
> Moreover, over-stuttering can hinder RCU's progress on TREE07 kernels.
> This happens as the stuttering code may run within a softirq due to RCU
> callbacks. Consequently, ksoftirqd keeps a CPU busy for several seconds,
> thus obstructing RCU's progress. This situation triggers a warning
> message in the logs:
> 
> [ 2169.481783] rcu_torture_writer: rtort_pipe_count: 9
> 
> This warning suggests that an RCU torture object, although invisible to
> RCU readers, couldn't make it past the pipe array and be freed -- a
> strong indication that there weren't enough grace periods during the
> stutter interval.
> 
> To address these issues, this patch sets the "stutter end" time to an
> absolute point in the future set by the main stutter thread. This is
> then used for waiting in stutter_wait(). While the stutter thread still
> defines this absolute time, the waiters' waiting logic doesn't rely on
> the stutter thread receiving sufficient CPU time to halt the stuttering
> as the halting is now self-controlled.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/torture.c | 46 +++++++++++++---------------------------------
>  1 file changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/torture.c b/kernel/torture.c
> index 68dba4ecab5c..63f8f2a7d960 100644
> --- a/kernel/torture.c
> +++ b/kernel/torture.c
> @@ -719,7 +719,7 @@ static void torture_shutdown_cleanup(void)
>   * suddenly applied to or removed from the system.
>   */
>  static struct task_struct *stutter_task;
> -static int stutter_pause_test;
> +static ktime_t stutter_till_abs_time;
>  static int stutter;
>  static int stutter_gap;
>  
> @@ -729,30 +729,17 @@ static int stutter_gap;
>   */
>  bool stutter_wait(const char *title)
>  {
> -	unsigned int i = 0;
>  	bool ret = false;
> -	int spt;
> +	ktime_t now_ns, till_ns;
>  
>  	cond_resched_tasks_rcu_qs();
> -	spt = READ_ONCE(stutter_pause_test);
> -	for (; spt; spt = READ_ONCE(stutter_pause_test)) {
> -		if (!ret && !rt_task(current)) {
> -			sched_set_normal(current, MAX_NICE);
> -			ret = true;
> -		}
> -		if (spt == 1) {
> -			torture_hrtimeout_jiffies(1, NULL);
> -		} else if (spt == 2) {
> -			while (READ_ONCE(stutter_pause_test)) {
> -				if (!(i++ & 0xffff))
> -					torture_hrtimeout_us(10, 0, NULL);
> -				cond_resched();
> -			}
> -		} else {
> -			torture_hrtimeout_jiffies(round_jiffies_relative(HZ), NULL);
> -		}
> -		torture_shutdown_absorb(title);
> +	now_ns = ktime_get();
> +	till_ns = READ_ONCE(stutter_till_abs_time);
> +	if (till_ns && ktime_before(now_ns, till_ns)) {
> +		torture_hrtimeout_ns(ktime_sub(till_ns, now_ns), 0, NULL);

This ktime_sub() is roughly cancelled out by a ktime_add_safe() in
__hrtimer_start_range_ns().  Perhaps torture_hrtimeout_ns() needs to
take a mode argument as in the patch at the end of this email, allowing
you to ditch that ktime_sub() in favor of HRTIMER_MODE_ABS.

							Thanx, Paul

> +		ret = true;
>  	}
> +	torture_shutdown_absorb(title);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stutter_wait);
> @@ -763,23 +750,16 @@ EXPORT_SYMBOL_GPL(stutter_wait);
>   */
>  static int torture_stutter(void *arg)
>  {
> -	DEFINE_TORTURE_RANDOM(rand);
> -	int wtime;
> +	ktime_t till_ns;
>  
>  	VERBOSE_TOROUT_STRING("torture_stutter task started");
>  	do {
>  		if (!torture_must_stop() && stutter > 1) {
> -			wtime = stutter;
> -			if (stutter > 2) {
> -				WRITE_ONCE(stutter_pause_test, 1);
> -				wtime = stutter - 3;
> -				torture_hrtimeout_jiffies(wtime, &rand);
> -				wtime = 2;
> -			}
> -			WRITE_ONCE(stutter_pause_test, 2);
> -			torture_hrtimeout_jiffies(wtime, NULL);
> +			till_ns = ktime_add_ns(ktime_get(),
> +					       jiffies_to_nsecs(stutter));
> +			WRITE_ONCE(stutter_till_abs_time, till_ns);
> +			torture_hrtimeout_jiffies(stutter - 1, NULL);
>  		}
> -		WRITE_ONCE(stutter_pause_test, 0);
>  		if (!torture_must_stop())
>  			torture_hrtimeout_jiffies(stutter_gap, NULL);
>  		torture_shutdown_absorb("torture_stutter");

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

commit 6445f014c3d4d20a0b69bd97d3b76a222820612f
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Wed Jul 26 13:57:03 2023 -0700

    torture: Make torture_hrtimeout_ns() take an hrtimer mode parameter
    
    The current torture-test sleeps are waiting for a duration, but there
    are situations where it is better to wait for an absolute time, for
    example, when ending a stutter interval.  This commit therefore adds
    an hrtimer mode parameter to torture_hrtimeout_ns().  Why not also the
    other torture_hrtimeout_*() functions?  The theory is that most absolute
    times will be in nanoseconds, especially not (say) jiffies.
    
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/include/linux/torture.h b/include/linux/torture.h
index bb466eec01e4..017f0f710815 100644
--- a/include/linux/torture.h
+++ b/include/linux/torture.h
@@ -81,7 +81,8 @@ static inline void torture_random_init(struct torture_random_state *trsp)
 }
 
 /* Definitions for high-resolution-timer sleeps. */
-int torture_hrtimeout_ns(ktime_t baset_ns, u32 fuzzt_ns, struct torture_random_state *trsp);
+int torture_hrtimeout_ns(ktime_t baset_ns, u32 fuzzt_ns, const enum hrtimer_mode mode,
+			 struct torture_random_state *trsp);
 int torture_hrtimeout_us(u32 baset_us, u32 fuzzt_ns, struct torture_random_state *trsp);
 int torture_hrtimeout_ms(u32 baset_ms, u32 fuzzt_us, struct torture_random_state *trsp);
 int torture_hrtimeout_jiffies(u32 baset_j, struct torture_random_state *trsp);
diff --git a/kernel/torture.c b/kernel/torture.c
index 68dba4ecab5c..6ba62e5993e7 100644
--- a/kernel/torture.c
+++ b/kernel/torture.c
@@ -87,14 +87,15 @@ EXPORT_SYMBOL_GPL(verbose_torout_sleep);
  * nanosecond random fuzz.  This function and its friends desynchronize
  * testing from the timer wheel.
  */
-int torture_hrtimeout_ns(ktime_t baset_ns, u32 fuzzt_ns, struct torture_random_state *trsp)
+int torture_hrtimeout_ns(ktime_t baset_ns, u32 fuzzt_ns, const enum hrtimer_mode mode,
+			 struct torture_random_state *trsp)
 {
 	ktime_t hto = baset_ns;
 
 	if (trsp)
 		hto += torture_random(trsp) % fuzzt_ns;
 	set_current_state(TASK_IDLE);
-	return schedule_hrtimeout(&hto, HRTIMER_MODE_REL);
+	return schedule_hrtimeout(&hto, mode);
 }
 EXPORT_SYMBOL_GPL(torture_hrtimeout_ns);
 
@@ -106,7 +107,7 @@ int torture_hrtimeout_us(u32 baset_us, u32 fuzzt_ns, struct torture_random_state
 {
 	ktime_t baset_ns = baset_us * NSEC_PER_USEC;
 
-	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, trsp);
+	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, HRTIMER_MODE_REL, trsp);
 }
 EXPORT_SYMBOL_GPL(torture_hrtimeout_us);
 
@@ -123,7 +124,7 @@ int torture_hrtimeout_ms(u32 baset_ms, u32 fuzzt_us, struct torture_random_state
 		fuzzt_ns = (u32)~0U;
 	else
 		fuzzt_ns = fuzzt_us * NSEC_PER_USEC;
-	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, trsp);
+	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, HRTIMER_MODE_REL, trsp);
 }
 EXPORT_SYMBOL_GPL(torture_hrtimeout_ms);
 
@@ -136,7 +137,7 @@ int torture_hrtimeout_jiffies(u32 baset_j, struct torture_random_state *trsp)
 {
 	ktime_t baset_ns = jiffies_to_nsecs(baset_j);
 
-	return torture_hrtimeout_ns(baset_ns, jiffies_to_nsecs(1), trsp);
+	return torture_hrtimeout_ns(baset_ns, jiffies_to_nsecs(1), HRTIMER_MODE_REL, trsp);
 }
 EXPORT_SYMBOL_GPL(torture_hrtimeout_jiffies);
 
@@ -153,7 +154,7 @@ int torture_hrtimeout_s(u32 baset_s, u32 fuzzt_ms, struct torture_random_state *
 		fuzzt_ns = (u32)~0U;
 	else
 		fuzzt_ns = fuzzt_ms * NSEC_PER_MSEC;
-	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, trsp);
+	return torture_hrtimeout_ns(baset_ns, fuzzt_ns, HRTIMER_MODE_REL, trsp);
 }
 EXPORT_SYMBOL_GPL(torture_hrtimeout_s);
 

Re: [PATCH 1/5] rcutorture: Fix stuttering races and other issues

[Joel Fernandes]

On Wed, Jul 26, 2023 at 4:59 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Tue, Jul 25, 2023 at 11:29:06PM +0000, Joel Fernandes (Google) wrote:
> > The stuttering code isn't functioning as expected. Ideally, it should
> > pause the torture threads for a designated period before resuming. Yet,
> > it fails to halt the test for the correct duration. Additionally, a race
> > condition exists, potentially causing the stuttering code to pause for
> > an extended period if the 'spt' variable is non-zero due to the stutter
> > orchestration thread's inadequate CPU time.
> >
> > Moreover, over-stuttering can hinder RCU's progress on TREE07 kernels.
> > This happens as the stuttering code may run within a softirq due to RCU
> > callbacks. Consequently, ksoftirqd keeps a CPU busy for several seconds,
> > thus obstructing RCU's progress. This situation triggers a warning
> > message in the logs:
> >
> > [ 2169.481783] rcu_torture_writer: rtort_pipe_count: 9
> >
> > This warning suggests that an RCU torture object, although invisible to
> > RCU readers, couldn't make it past the pipe array and be freed -- a
> > strong indication that there weren't enough grace periods during the
> > stutter interval.
> >
> > To address these issues, this patch sets the "stutter end" time to an
> > absolute point in the future set by the main stutter thread. This is
> > then used for waiting in stutter_wait(). While the stutter thread still
> > defines this absolute time, the waiters' waiting logic doesn't rely on
> > the stutter thread receiving sufficient CPU time to halt the stuttering
> > as the halting is now self-controlled.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/torture.c | 46 +++++++++++++---------------------------------
> >  1 file changed, 13 insertions(+), 33 deletions(-)
> >
> > diff --git a/kernel/torture.c b/kernel/torture.c
> > index 68dba4ecab5c..63f8f2a7d960 100644
> > --- a/kernel/torture.c
> > +++ b/kernel/torture.c
> > @@ -719,7 +719,7 @@ static void torture_shutdown_cleanup(void)
> >   * suddenly applied to or removed from the system.
> >   */
> >  static struct task_struct *stutter_task;
> > -static int stutter_pause_test;
> > +static ktime_t stutter_till_abs_time;
> >  static int stutter;
> >  static int stutter_gap;
> >
> > @@ -729,30 +729,17 @@ static int stutter_gap;
> >   */
> >  bool stutter_wait(const char *title)
> >  {
> > -     unsigned int i = 0;
> >       bool ret = false;
> > -     int spt;
> > +     ktime_t now_ns, till_ns;
> >
> >       cond_resched_tasks_rcu_qs();
> > -     spt = READ_ONCE(stutter_pause_test);
> > -     for (; spt; spt = READ_ONCE(stutter_pause_test)) {
> > -             if (!ret && !rt_task(current)) {
> > -                     sched_set_normal(current, MAX_NICE);
> > -                     ret = true;
> > -             }
> > -             if (spt == 1) {
> > -                     torture_hrtimeout_jiffies(1, NULL);
> > -             } else if (spt == 2) {
> > -                     while (READ_ONCE(stutter_pause_test)) {
> > -                             if (!(i++ & 0xffff))
> > -                                     torture_hrtimeout_us(10, 0, NULL);
> > -                             cond_resched();
> > -                     }
> > -             } else {
> > -                     torture_hrtimeout_jiffies(round_jiffies_relative(HZ), NULL);
> > -             }
> > -             torture_shutdown_absorb(title);
> > +     now_ns = ktime_get();
> > +     till_ns = READ_ONCE(stutter_till_abs_time);
> > +     if (till_ns && ktime_before(now_ns, till_ns)) {
> > +             torture_hrtimeout_ns(ktime_sub(till_ns, now_ns), 0, NULL);
>
> This ktime_sub() is roughly cancelled out by a ktime_add_safe() in
> __hrtimer_start_range_ns().

Yes, functionally it is the same but your suggestion is more robust I think.

> Perhaps torture_hrtimeout_ns() needs to
> take a mode argument as in the patch at the end of this email, allowing
> you to ditch that ktime_sub() in favor of HRTIMER_MODE_ABS.

Sure, or we can add a new API and keep the default as relative?

Or have 2 APIs:
torture_hrtimeout_relative_ns();

and:
torture_hrtimeout_absolute_ns();

That makes it more readable IMHO.

Also, do you want me to make both changes (API and usage) in the same
patch? Or were you planning to have a separate patch yourself in -dev
which I can use? Let me know either way, and then I'll refresh the
patch.

thanks,

 - Joel

[..]

Re: [PATCH 1/5] rcutorture: Fix stuttering races and other issues

[Paul E. McKenney]

On Wed, Jul 26, 2023 at 11:01:40PM -0400, Joel Fernandes wrote:
> On Wed, Jul 26, 2023 at 4:59 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Tue, Jul 25, 2023 at 11:29:06PM +0000, Joel Fernandes (Google) wrote:
> > > The stuttering code isn't functioning as expected. Ideally, it should
> > > pause the torture threads for a designated period before resuming. Yet,
> > > it fails to halt the test for the correct duration. Additionally, a race
> > > condition exists, potentially causing the stuttering code to pause for
> > > an extended period if the 'spt' variable is non-zero due to the stutter
> > > orchestration thread's inadequate CPU time.
> > >
> > > Moreover, over-stuttering can hinder RCU's progress on TREE07 kernels.
> > > This happens as the stuttering code may run within a softirq due to RCU
> > > callbacks. Consequently, ksoftirqd keeps a CPU busy for several seconds,
> > > thus obstructing RCU's progress. This situation triggers a warning
> > > message in the logs:
> > >
> > > [ 2169.481783] rcu_torture_writer: rtort_pipe_count: 9
> > >
> > > This warning suggests that an RCU torture object, although invisible to
> > > RCU readers, couldn't make it past the pipe array and be freed -- a
> > > strong indication that there weren't enough grace periods during the
> > > stutter interval.
> > >
> > > To address these issues, this patch sets the "stutter end" time to an
> > > absolute point in the future set by the main stutter thread. This is
> > > then used for waiting in stutter_wait(). While the stutter thread still
> > > defines this absolute time, the waiters' waiting logic doesn't rely on
> > > the stutter thread receiving sufficient CPU time to halt the stuttering
> > > as the halting is now self-controlled.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  kernel/torture.c | 46 +++++++++++++---------------------------------
> > >  1 file changed, 13 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/kernel/torture.c b/kernel/torture.c
> > > index 68dba4ecab5c..63f8f2a7d960 100644
> > > --- a/kernel/torture.c
> > > +++ b/kernel/torture.c
> > > @@ -719,7 +719,7 @@ static void torture_shutdown_cleanup(void)
> > >   * suddenly applied to or removed from the system.
> > >   */
> > >  static struct task_struct *stutter_task;
> > > -static int stutter_pause_test;
> > > +static ktime_t stutter_till_abs_time;
> > >  static int stutter;
> > >  static int stutter_gap;
> > >
> > > @@ -729,30 +729,17 @@ static int stutter_gap;
> > >   */
> > >  bool stutter_wait(const char *title)
> > >  {
> > > -     unsigned int i = 0;
> > >       bool ret = false;
> > > -     int spt;
> > > +     ktime_t now_ns, till_ns;
> > >
> > >       cond_resched_tasks_rcu_qs();
> > > -     spt = READ_ONCE(stutter_pause_test);
> > > -     for (; spt; spt = READ_ONCE(stutter_pause_test)) {
> > > -             if (!ret && !rt_task(current)) {
> > > -                     sched_set_normal(current, MAX_NICE);
> > > -                     ret = true;
> > > -             }
> > > -             if (spt == 1) {
> > > -                     torture_hrtimeout_jiffies(1, NULL);
> > > -             } else if (spt == 2) {
> > > -                     while (READ_ONCE(stutter_pause_test)) {
> > > -                             if (!(i++ & 0xffff))
> > > -                                     torture_hrtimeout_us(10, 0, NULL);
> > > -                             cond_resched();
> > > -                     }
> > > -             } else {
> > > -                     torture_hrtimeout_jiffies(round_jiffies_relative(HZ), NULL);
> > > -             }
> > > -             torture_shutdown_absorb(title);
> > > +     now_ns = ktime_get();
> > > +     till_ns = READ_ONCE(stutter_till_abs_time);
> > > +     if (till_ns && ktime_before(now_ns, till_ns)) {
> > > +             torture_hrtimeout_ns(ktime_sub(till_ns, now_ns), 0, NULL);
> >
> > This ktime_sub() is roughly cancelled out by a ktime_add_safe() in
> > __hrtimer_start_range_ns().
> 
> Yes, functionally it is the same but your suggestion is more robust I think.
> 
> > Perhaps torture_hrtimeout_ns() needs to
> > take a mode argument as in the patch at the end of this email, allowing
> > you to ditch that ktime_sub() in favor of HRTIMER_MODE_ABS.
> 
> Sure, or we can add a new API and keep the default as relative?
> 
> Or have 2 APIs:
> torture_hrtimeout_relative_ns();
> 
> and:
> torture_hrtimeout_absolute_ns();
> 
> That makes it more readable IMHO.
> 
> Also, do you want me to make both changes (API and usage) in the same
> patch? Or were you planning to have a separate patch yourself in -dev
> which I can use? Let me know either way, and then I'll refresh the
> patch.

I queued the patch on the -rcu tree's "dev" branch.  It turns out that
torture_hrtimeout_ns() isn't called very many times, so adding the
parameter was straightforward.  Plus the compiler might well optimize
it away anyway.

							Thanx, Paul

Re: [PATCH 1/5] rcutorture: Fix stuttering races and other issues

[Joel Fernandes]

On 7/27/23 00:01, Paul E. McKenney wrote:
> On Wed, Jul 26, 2023 at 11:01:40PM -0400, Joel Fernandes wrote:
>> On Wed, Jul 26, 2023 at 4:59 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>>>
>>> On Tue, Jul 25, 2023 at 11:29:06PM +0000, Joel Fernandes (Google) wrote:
>>>> The stuttering code isn't functioning as expected. Ideally, it should
>>>> pause the torture threads for a designated period before resuming. Yet,
>>>> it fails to halt the test for the correct duration. Additionally, a race
>>>> condition exists, potentially causing the stuttering code to pause for
>>>> an extended period if the 'spt' variable is non-zero due to the stutter
>>>> orchestration thread's inadequate CPU time.
>>>>
>>>> Moreover, over-stuttering can hinder RCU's progress on TREE07 kernels.
>>>> This happens as the stuttering code may run within a softirq due to RCU
>>>> callbacks. Consequently, ksoftirqd keeps a CPU busy for several seconds,
>>>> thus obstructing RCU's progress. This situation triggers a warning
>>>> message in the logs:
>>>>
>>>> [ 2169.481783] rcu_torture_writer: rtort_pipe_count: 9
>>>>
>>>> This warning suggests that an RCU torture object, although invisible to
>>>> RCU readers, couldn't make it past the pipe array and be freed -- a
>>>> strong indication that there weren't enough grace periods during the
>>>> stutter interval.
>>>>
>>>> To address these issues, this patch sets the "stutter end" time to an
>>>> absolute point in the future set by the main stutter thread. This is
>>>> then used for waiting in stutter_wait(). While the stutter thread still
>>>> defines this absolute time, the waiters' waiting logic doesn't rely on
>>>> the stutter thread receiving sufficient CPU time to halt the stuttering
>>>> as the halting is now self-controlled.
>>>>
>>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>>> ---
>>>>   kernel/torture.c | 46 +++++++++++++---------------------------------
>>>>   1 file changed, 13 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/kernel/torture.c b/kernel/torture.c
>>>> index 68dba4ecab5c..63f8f2a7d960 100644
>>>> --- a/kernel/torture.c
>>>> +++ b/kernel/torture.c
>>>> @@ -719,7 +719,7 @@ static void torture_shutdown_cleanup(void)
>>>>    * suddenly applied to or removed from the system.
>>>>    */
>>>>   static struct task_struct *stutter_task;
>>>> -static int stutter_pause_test;
>>>> +static ktime_t stutter_till_abs_time;
>>>>   static int stutter;
>>>>   static int stutter_gap;
>>>>
>>>> @@ -729,30 +729,17 @@ static int stutter_gap;
>>>>    */
>>>>   bool stutter_wait(const char *title)
>>>>   {
>>>> -     unsigned int i = 0;
>>>>        bool ret = false;
>>>> -     int spt;
>>>> +     ktime_t now_ns, till_ns;
>>>>
>>>>        cond_resched_tasks_rcu_qs();
>>>> -     spt = READ_ONCE(stutter_pause_test);
>>>> -     for (; spt; spt = READ_ONCE(stutter_pause_test)) {
>>>> -             if (!ret && !rt_task(current)) {
>>>> -                     sched_set_normal(current, MAX_NICE);
>>>> -                     ret = true;
>>>> -             }
>>>> -             if (spt == 1) {
>>>> -                     torture_hrtimeout_jiffies(1, NULL);
>>>> -             } else if (spt == 2) {
>>>> -                     while (READ_ONCE(stutter_pause_test)) {
>>>> -                             if (!(i++ & 0xffff))
>>>> -                                     torture_hrtimeout_us(10, 0, NULL);
>>>> -                             cond_resched();
>>>> -                     }
>>>> -             } else {
>>>> -                     torture_hrtimeout_jiffies(round_jiffies_relative(HZ), NULL);
>>>> -             }
>>>> -             torture_shutdown_absorb(title);
>>>> +     now_ns = ktime_get();
>>>> +     till_ns = READ_ONCE(stutter_till_abs_time);
>>>> +     if (till_ns && ktime_before(now_ns, till_ns)) {
>>>> +             torture_hrtimeout_ns(ktime_sub(till_ns, now_ns), 0, NULL);
>>>
>>> This ktime_sub() is roughly cancelled out by a ktime_add_safe() in
>>> __hrtimer_start_range_ns().
>>
>> Yes, functionally it is the same but your suggestion is more robust I think.
>>
>>> Perhaps torture_hrtimeout_ns() needs to
>>> take a mode argument as in the patch at the end of this email, allowing
>>> you to ditch that ktime_sub() in favor of HRTIMER_MODE_ABS.
>>
>> Sure, or we can add a new API and keep the default as relative?
>>
>> Or have 2 APIs:
>> torture_hrtimeout_relative_ns();
>>
>> and:
>> torture_hrtimeout_absolute_ns();
>>
>> That makes it more readable IMHO.
>>
>> Also, do you want me to make both changes (API and usage) in the same
>> patch? Or were you planning to have a separate patch yourself in -dev
>> which I can use? Let me know either way, and then I'll refresh the
>> patch.
> 
> I queued the patch on the -rcu tree's "dev" branch.  It turns out that
> torture_hrtimeout_ns() isn't called very many times, so adding the
> parameter was straightforward.  Plus the compiler might well optimize
> it away anyway.

Ok sounds good, I will make use of it in this patch and send it again after testing.

thanks,

  - Joel

[PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()

[Joel Fernandes (Google)]

The current error handling in init_srcu_struct_fields() is a bit
inconsistent.  If init_srcu_struct_nodes() fails, the function either
returns -ENOMEM or 0 depending on whether ssp->sda_is_static is true or
false. This can make init_srcu_struct_fields() return 0 even if memory
allocation failed!

Simplify the error handling by always returning -ENOMEM if either
init_srcu_struct_nodes() or the per-CPU allocation fails. This makes the
control flow easier to follow and avoids the inconsistent return values.

Add goto labels to avoid duplicating the error cleanup code.

Link: https://lore.kernel.org/r/20230404003508.GA254019@google.com
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/srcutree.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 20d7a238d675..cbc37cbc1805 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -255,29 +255,32 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
 	ssp->srcu_sup->sda_is_static = is_static;
 	if (!is_static)
 		ssp->sda = alloc_percpu(struct srcu_data);
-	if (!ssp->sda) {
-		if (!is_static)
-			kfree(ssp->srcu_sup);
-		return -ENOMEM;
-	}
+	if (!ssp->sda)
+		goto err_free_sup;
 	init_srcu_struct_data(ssp);
 	ssp->srcu_sup->srcu_gp_seq_needed_exp = 0;
 	ssp->srcu_sup->srcu_last_gp_end = ktime_get_mono_fast_ns();
 	if (READ_ONCE(ssp->srcu_sup->srcu_size_state) == SRCU_SIZE_SMALL && SRCU_SIZING_IS_INIT()) {
-		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC)) {
-			if (!ssp->srcu_sup->sda_is_static) {
-				free_percpu(ssp->sda);
-				ssp->sda = NULL;
-				kfree(ssp->srcu_sup);
-				return -ENOMEM;
-			}
-		} else {
+		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
+			goto err_free_sda;
+		else
 			WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
-		}
 	}
 	ssp->srcu_sup->srcu_ssp = ssp;
 	smp_store_release(&ssp->srcu_sup->srcu_gp_seq_needed, 0); /* Init done. */
 	return 0;
+
+err_free_sda:
+	if (!is_static) {
+		free_percpu(ssp->sda);
+		ssp->sda = NULL;
+	}
+err_free_sup:
+	if (!is_static) {
+		kfree(ssp->srcu_sup);
+		ssp->srcu_sup = NULL;
+	}
+	return -ENOMEM;
 }
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()

[Paul E. McKenney]

On Tue, Jul 25, 2023 at 11:29:07PM +0000, Joel Fernandes (Google) wrote:
> The current error handling in init_srcu_struct_fields() is a bit
> inconsistent.  If init_srcu_struct_nodes() fails, the function either
> returns -ENOMEM or 0 depending on whether ssp->sda_is_static is true or
> false. This can make init_srcu_struct_fields() return 0 even if memory
> allocation failed!
> 
> Simplify the error handling by always returning -ENOMEM if either
> init_srcu_struct_nodes() or the per-CPU allocation fails. This makes the
> control flow easier to follow and avoids the inconsistent return values.
> 
> Add goto labels to avoid duplicating the error cleanup code.
> 
> Link: https://lore.kernel.org/r/20230404003508.GA254019@google.com
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Looks good, nice simplification!  One nit below.

							Thanx, Paul

> ---
>  kernel/rcu/srcutree.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 20d7a238d675..cbc37cbc1805 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -255,29 +255,32 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
>  	ssp->srcu_sup->sda_is_static = is_static;
>  	if (!is_static)
>  		ssp->sda = alloc_percpu(struct srcu_data);
> -	if (!ssp->sda) {
> -		if (!is_static)
> -			kfree(ssp->srcu_sup);
> -		return -ENOMEM;
> -	}
> +	if (!ssp->sda)
> +		goto err_free_sup;
>  	init_srcu_struct_data(ssp);
>  	ssp->srcu_sup->srcu_gp_seq_needed_exp = 0;
>  	ssp->srcu_sup->srcu_last_gp_end = ktime_get_mono_fast_ns();
>  	if (READ_ONCE(ssp->srcu_sup->srcu_size_state) == SRCU_SIZE_SMALL && SRCU_SIZING_IS_INIT()) {
> -		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC)) {
> -			if (!ssp->srcu_sup->sda_is_static) {

I was going to complain about this ssp->srcu_sup->sda_is_static becoming
just is_static, but now I cannot see why I didn't just use is_static in
the first place.  ;-)

> -				free_percpu(ssp->sda);
> -				ssp->sda = NULL;
> -				kfree(ssp->srcu_sup);
> -				return -ENOMEM;
> -			}
> -		} else {
> +		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
> +			goto err_free_sda;
> +		else
>  			WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);

Given that the "then" clause is a goto, what is the "else" clause doing
for us?

> -		}
>  	}
>  	ssp->srcu_sup->srcu_ssp = ssp;
>  	smp_store_release(&ssp->srcu_sup->srcu_gp_seq_needed, 0); /* Init done. */
>  	return 0;
> +
> +err_free_sda:
> +	if (!is_static) {
> +		free_percpu(ssp->sda);
> +		ssp->sda = NULL;
> +	}
> +err_free_sup:
> +	if (!is_static) {
> +		kfree(ssp->srcu_sup);
> +		ssp->srcu_sup = NULL;
> +	}
> +	return -ENOMEM;
>  }
>  
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> -- 
> 2.41.0.487.g6d72f3e995-goog
> 

Re: [PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()

[Joel Fernandes]

On 7/26/23 17:07, Paul E. McKenney wrote:
>> -				free_percpu(ssp->sda);
>> -				ssp->sda = NULL;
>> -				kfree(ssp->srcu_sup);
>> -				return -ENOMEM;
>> -			}
>> -		} else {
>> +		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
>> +			goto err_free_sda;
>> +		else
>>  			WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
> Given that the "then" clause is a goto, what is the "else" clause doing
> for us?
> 

Not much. Agreed we can get rid of "else" and reduce indent of the
WRITE_ONCE that follows.

Would you mind making this fixup in the patch for your apply, or would
you like me to refresh the patch? Let me know either way, thank you!

 - Joel

Re: [PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()

[Paul E. McKenney]

On Wed, Jul 26, 2023 at 11:04:04PM -0400, Joel Fernandes wrote:
> 
> 
> On 7/26/23 17:07, Paul E. McKenney wrote:
> >> -				free_percpu(ssp->sda);
> >> -				ssp->sda = NULL;
> >> -				kfree(ssp->srcu_sup);
> >> -				return -ENOMEM;
> >> -			}
> >> -		} else {
> >> +		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
> >> +			goto err_free_sda;
> >> +		else
> >>  			WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
> > Given that the "then" clause is a goto, what is the "else" clause doing
> > for us?
> > 
> 
> Not much. Agreed we can get rid of "else" and reduce indent of the
> WRITE_ONCE that follows.
> 
> Would you mind making this fixup in the patch for your apply, or would
> you like me to refresh the patch? Let me know either way, thank you!

Please include it with your next series, which has at least one other
change anyway.  ;-)

							Thanx, Paul

Re: [PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()

[Joel Fernandes]

> On Jul 27, 2023, at 12:02 AM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Wed, Jul 26, 2023 at 11:04:04PM -0400, Joel Fernandes wrote:
>> 
>> 
>> On 7/26/23 17:07, Paul E. McKenney wrote:
>>>> -                free_percpu(ssp->sda);
>>>> -                ssp->sda = NULL;
>>>> -                kfree(ssp->srcu_sup);
>>>> -                return -ENOMEM;
>>>> -            }
>>>> -        } else {
>>>> +        if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
>>>> +            goto err_free_sda;
>>>> +        else
>>>>            WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
>>> Given that the "then" clause is a goto, what is the "else" clause doing
>>> for us?
>>> 
>> 
>> Not much. Agreed we can get rid of "else" and reduce indent of the
>> WRITE_ONCE that follows.
>> 
>> Would you mind making this fixup in the patch for your apply, or would
>> you like me to refresh the patch? Let me know either way, thank you!
> 
> Please include it with your next series, which has at least one other
> change anyway.  ;-)

My pleasure ;-).

- Joel


> 
>                            Thanx, Paul

[PATCH 3/5] tree/nocb: Adjust RCU_NOCB_WAKE_* macros from weaker to stronger

[Joel Fernandes (Google)]

This is needed to make the next patch work correctly as we rely on the
strength of the wakeup when comparing deferred-wakeup types across
different CPUs.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 192536916f9a..0f40a9c2b78d 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -288,8 +288,8 @@ struct rcu_data {
 
 /* Values for nocb_defer_wakeup field in struct rcu_data. */
 #define RCU_NOCB_WAKE_NOT	0
-#define RCU_NOCB_WAKE_BYPASS	1
-#define RCU_NOCB_WAKE_LAZY	2
+#define RCU_NOCB_WAKE_LAZY	1
+#define RCU_NOCB_WAKE_BYPASS	2
 #define RCU_NOCB_WAKE		3
 #define RCU_NOCB_WAKE_FORCE	4
 
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 4/5] tree/nocb: Improve readability of nocb_gp_wait()

[Joel Fernandes (Google)]

The nocb_gp_wait() function contains logic to check each rdp's bypass
list, flush if needed, and decide on wakeups. This makes the function
hard to follow.

Split out the bypass checking and flushing into a separate helper
nocb_gp_flush_wake(). The new function encapsulates the logic to:

- Check if the bypass needs to be flushed
- Flush if needed
- Return info on wakeups (lazy, bypass or none)

nocb_gp_wait() now becomes simpler by calling the helper to handle
the bypass flushing and waking logic.

This splitting improves the readability and maintainability of the
code by encapsulating related logic into a function with a clear
purpose.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree_nocb.h | 113 ++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 47 deletions(-)

diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
index 5598212d1f27..c805825c3f00 100644
--- a/kernel/rcu/tree_nocb.h
+++ b/kernel/rcu/tree_nocb.h
@@ -670,22 +670,68 @@ static void nocb_gp_sleep(struct rcu_data *my_rdp, int cpu)
 	trace_rcu_nocb_wake(rcu_state.name, cpu, TPS("EndSleep"));
 }
 
+/*
+ * Given an rdp, flush its bypass list if needed and return information about
+ * if a deferred-wakeup needs to be organized depending on whether things are
+ * still in the bypass list. Also tell caller if the list was flushed and if it
+ * is still empty after any flushing.
+ */
+static int nocb_gp_flush_wake(struct rcu_data *rdp, bool *empty, bool *flush)
+{
+	long bypass_ncbs;
+	long lazy_ncbs;
+	unsigned long j = jiffies;
+
+	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("Check"));
+	lockdep_assert_held(&rdp->nocb_lock);
+	bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
+	lazy_ncbs = READ_ONCE(rdp->lazy_len);
+
+	*flush = false;
+	*empty = false;
+	if (bypass_ncbs && (lazy_ncbs == bypass_ncbs) &&
+	    (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + jiffies_till_flush) ||
+	     bypass_ncbs > 2 * qhimark)) {
+		*flush = true;
+	} else if (bypass_ncbs && (lazy_ncbs != bypass_ncbs) &&
+			(time_after(j, READ_ONCE(rdp->nocb_bypass_first) + 1) ||
+			 bypass_ncbs > 2 * qhimark)) {
+		*flush = true;
+	} else if (!bypass_ncbs && rcu_segcblist_empty(&rdp->cblist)) {
+		*empty = true;
+		return RCU_NOCB_WAKE_NOT;
+	}
+
+	if (*flush) {
+		// Bypass full or old, so flush it.
+		(void)rcu_nocb_try_flush_bypass(rdp, j);
+		bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
+		lazy_ncbs = READ_ONCE(rdp->lazy_len);
+	}
+
+	if (bypass_ncbs) {
+		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
+				    bypass_ncbs == lazy_ncbs ? TPS("Lazy") : TPS("Bypass"));
+		return (bypass_ncbs == lazy_ncbs ? RCU_NOCB_WAKE_LAZY :
+						   RCU_NOCB_WAKE_BYPASS);
+	}
+	return RCU_NOCB_WAKE_NOT;
+}
+
 /*
  * No-CBs GP kthreads come here to wait for additional callbacks to show up
  * or for grace periods to end.
  */
 static void nocb_gp_wait(struct rcu_data *my_rdp)
 {
-	bool bypass = false;
 	int __maybe_unused cpu = my_rdp->cpu;
 	unsigned long cur_gp_seq;
 	unsigned long flags;
 	bool gotcbs = false;
-	unsigned long j = jiffies;
-	bool lazy = false;
 	bool needwait_gp = false; // This prevents actual uninitialized use.
 	bool needwake;
 	bool needwake_gp;
+	int defer_wake_type = RCU_NOCB_WAKE_NOT;
 	struct rcu_data *rdp, *rdp_toggling = NULL;
 	struct rcu_node *rnp;
 	unsigned long wait_gp_seq = 0; // Suppress "use uninitialized" warning.
@@ -712,44 +758,24 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 	 * won't be ignored for long.
 	 */
 	list_for_each_entry(rdp, &my_rdp->nocb_head_rdp, nocb_entry_rdp) {
-		long bypass_ncbs;
-		bool flush_bypass = false;
-		long lazy_ncbs;
+		int defer_wake_type_one = RCU_NOCB_WAKE_NOT;
+		bool flushed;
+		bool empty;
 
-		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("Check"));
 		rcu_nocb_lock_irqsave(rdp, flags);
-		lockdep_assert_held(&rdp->nocb_lock);
-		bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
-		lazy_ncbs = READ_ONCE(rdp->lazy_len);
+		defer_wake_type_one = nocb_gp_flush_wake(rdp, &empty, &flushed);
 
-		if (bypass_ncbs && (lazy_ncbs == bypass_ncbs) &&
-		    (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + jiffies_till_flush) ||
-		     bypass_ncbs > 2 * qhimark)) {
-			flush_bypass = true;
-		} else if (bypass_ncbs && (lazy_ncbs != bypass_ncbs) &&
-		    (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + 1) ||
-		     bypass_ncbs > 2 * qhimark)) {
-			flush_bypass = true;
-		} else if (!bypass_ncbs && rcu_segcblist_empty(&rdp->cblist)) {
-			rcu_nocb_unlock_irqrestore(rdp, flags);
-			continue; /* No callbacks here, try next. */
-		}
+		// We may need to do a deferred wakeup later for bypass/lazy
+		// So note down what we learnt from the rdp.
+		defer_wake_type = max(defer_wake_type_one, defer_wake_type);
 
-		if (flush_bypass) {
-			// Bypass full or old, so flush it.
-			(void)rcu_nocb_try_flush_bypass(rdp, j);
-			bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
-			lazy_ncbs = READ_ONCE(rdp->lazy_len);
+		// Did we make any updates to main cblist? If not, no
+		// non-deferred wake up to do for this rdp.
+		if (!flushed && empty) {
+			rcu_nocb_unlock_irqrestore(rdp, flags);
+			continue;
 		}
 
-		if (bypass_ncbs) {
-			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
-					    bypass_ncbs == lazy_ncbs ? TPS("Lazy") : TPS("Bypass"));
-			if (bypass_ncbs == lazy_ncbs)
-				lazy = true;
-			else
-				bypass = true;
-		}
 		rnp = rdp->mynode;
 
 		// Advance callbacks if helpful and low contention.
@@ -792,23 +818,16 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
 			rcu_gp_kthread_wake();
 	}
 
-	my_rdp->nocb_gp_bypass = bypass;
+	my_rdp->nocb_gp_bypass = (defer_wake_type == RCU_NOCB_WAKE_BYPASS);
 	my_rdp->nocb_gp_gp = needwait_gp;
 	my_rdp->nocb_gp_seq = needwait_gp ? wait_gp_seq : 0;
 
 	// At least one child with non-empty ->nocb_bypass, so set
 	// timer in order to avoid stranding its callbacks.
-	if (!rcu_nocb_poll) {
-		// If bypass list only has lazy CBs. Add a deferred lazy wake up.
-		if (lazy && !bypass) {
-			wake_nocb_gp_defer(my_rdp, RCU_NOCB_WAKE_LAZY,
-					TPS("WakeLazyIsDeferred"));
-		// Otherwise add a deferred bypass wake up.
-		} else if (bypass) {
-			wake_nocb_gp_defer(my_rdp, RCU_NOCB_WAKE_BYPASS,
-					TPS("WakeBypassIsDeferred"));
-		}
-	}
+	if (!rcu_nocb_poll && defer_wake_type != RCU_NOCB_WAKE_NOT)
+		wake_nocb_gp_defer(my_rdp, defer_wake_type,
+				   defer_wake_type == RCU_NOCB_WAKE_LAZY ?
+				   TPS("WakeLazyIsDeferred") : TPS("WakeBypassIsDeferred"));
 
 	if (rcu_nocb_poll) {
 		/* Polling, so trace if first poll in the series. */
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 5/5] Revert "checkpatch: Error out if deprecated RCU API used"

[Joel Fernandes (Google)]

The check for single-argument kfree_rcu() is no longer needed as all
users have been converted.

This reverts commit 1eacac3255495be7502d406e2ba5444fb5c3607c.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 scripts/checkpatch.pl | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9841148cde2..528f619520eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6422,15 +6422,6 @@ sub process {
 			}
 		}
 
-# check for soon-to-be-deprecated single-argument k[v]free_rcu() API
-		if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) {
-			if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) {
-				ERROR("DEPRECATED_API",
-				      "Single-argument k[v]free_rcu() API is deprecated, please pass rcu_head object or call k[v]free_rcu_mightsleep()." . $herecurr);
-			}
-		}
-
-
 # check for unnecessary "Out of Memory" messages
 		if ($line =~ /^\+.*\b$logFunctions\s*\(/ &&
 		    $prevline =~ /^[ \+]\s*if\s*\(\s*(\!\s*|NULL\s*==\s*)?($Lval)(\s*==\s*NULL\s*)?\s*\)/ &&
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 5/5] Revert "checkpatch: Error out if deprecated RCU API used"

[Paul E. McKenney]

On Tue, Jul 25, 2023 at 11:29:10PM +0000, Joel Fernandes (Google) wrote:
> The check for single-argument kfree_rcu() is no longer needed as all
> users have been converted.
> 
> This reverts commit 1eacac3255495be7502d406e2ba5444fb5c3607c.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Good eyes, thank you!

I applied this with an updated commit log as shown below.  Please let
me know if I messed something up.

							Thanx, Paul

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

commit 1f5f0053f7662385d9ead46d4c0d2d4fe9aaa73a
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date:   Tue Jul 25 23:29:10 2023 +0000

    Revert "checkpatch: Error out if deprecated RCU API used"
    
    The definition for single-argument kfree_rcu() has been removed,
    so that any further attempt to use it will result in a build error.
    Because of this build error, there is no longer any need for a special
    check in checkpatch.pl.
    
    Therefore, revert commit 1eacac3255495be7502d406e2ba5444fb5c3607c.
    
    Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9841148cde2..528f619520eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6422,15 +6422,6 @@ sub process {
 			}
 		}
 
-# check for soon-to-be-deprecated single-argument k[v]free_rcu() API
-		if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) {
-			if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) {
-				ERROR("DEPRECATED_API",
-				      "Single-argument k[v]free_rcu() API is deprecated, please pass rcu_head object or call k[v]free_rcu_mightsleep()." . $herecurr);
-			}
-		}
-
-
 # check for unnecessary "Out of Memory" messages
 		if ($line =~ /^\+.*\b$logFunctions\s*\(/ &&
 		    $prevline =~ /^[ \+]\s*if\s*\(\s*(\!\s*|NULL\s*==\s*)?($Lval)(\s*==\s*NULL\s*)?\s*\)/ &&

Re: [PATCH 5/5] Revert "checkpatch: Error out if deprecated RCU API used"

[Joel Fernandes]

On Wed, Jul 26, 2023 at 5:12 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Tue, Jul 25, 2023 at 11:29:10PM +0000, Joel Fernandes (Google) wrote:
> > The check for single-argument kfree_rcu() is no longer needed as all
> > users have been converted.
> >
> > This reverts commit 1eacac3255495be7502d406e2ba5444fb5c3607c.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Good eyes, thank you!
>
> I applied this with an updated commit log as shown below.  Please let
> me know if I messed something up.

LGTM. Thank you!

 - Joel


>
>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 1f5f0053f7662385d9ead46d4c0d2d4fe9aaa73a
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date:   Tue Jul 25 23:29:10 2023 +0000
>
>     Revert "checkpatch: Error out if deprecated RCU API used"
>
>     The definition for single-argument kfree_rcu() has been removed,
>     so that any further attempt to use it will result in a build error.
>     Because of this build error, there is no longer any need for a special
>     check in checkpatch.pl.
>
>     Therefore, revert commit 1eacac3255495be7502d406e2ba5444fb5c3607c.
>
>     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a9841148cde2..528f619520eb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6422,15 +6422,6 @@ sub process {
>                         }
>                 }
>
> -# check for soon-to-be-deprecated single-argument k[v]free_rcu() API
> -               if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) {
> -                       if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) {
> -                               ERROR("DEPRECATED_API",
> -                                     "Single-argument k[v]free_rcu() API is deprecated, please pass rcu_head object or call k[v]free_rcu_mightsleep()." . $herecurr);
> -                       }
> -               }
> -
> -
>  # check for unnecessary "Out of Memory" messages
>                 if ($line =~ /^\+.*\b$logFunctions\s*\(/ &&
>                     $prevline =~ /^[ \+]\s*if\s*\(\s*(\!\s*|NULL\s*==\s*)?($Lval)(\s*==\s*NULL\s*)?\s*\)/ &&