Message-ID: <98e35c90-5df3-4841-b5b9-7e8d18bab4f8@paulmck-laptop>
Date: Thu, 10 Apr 2025 11:54:13 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Joel Fernandes <joelagnelf@...dia.com>
Cc: linux-kernel@...r.kernel.org, Frederic Weisbecker <frederic@...nel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
	Joel Fernandes <joel@...lfernandes.org>,
	Josh Triplett <josh@...htriplett.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Uladzislau Rezki <urezki@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Zqiang <qiang.zhang1211@...il.com>,
	Davidlohr Bueso <dave@...olabs.net>, rcu@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rcutorture: Perform more frequent testing of
 ->gpwrap

On Thu, Apr 10, 2025 at 11:29:03AM -0700, Paul E. McKenney wrote:
> On Thu, Apr 10, 2025 at 11:03:27AM -0400, Joel Fernandes wrote: >
> Currently, the ->gpwrap is not tested (at all per my testing) due to
> the > requirement of a large delta between a CPU's rdp->gp_seq and its
> node's > rnp->gpseq.  > > This results in no testing of ->gpwrap being
> set. This patch by default > adds 5 minutes of testing with ->gpwrap
> forced by lowering the delta > between rdp->gp_seq and rnp->gp_seq to
> just 8 GPs. All of this is > configurable, including the active time for
> the setting and a full > testing cycle.  > > By default, the first 25
> minutes of a test will have the _default_ > behavior there is right now
> (ULONG_MAX / 4) delta. Then for 5 minutes, > we switch to a smaller delta
> causing 1-2 wraps in 5 minutes. I believe > this is reasonable since we
> at least add a little bit of testing for > usecases where ->gpwrap is set.
> > > Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> 
> Much better, thank you!
> 
> One potential nit below.  I will run some tests on this version.

And please feel free to apply the following to both:

Tested-by: Paul E. McKenney <paulmck@...nel.org>

> > ---
> >  kernel/rcu/rcu.h        |  4 +++
> >  kernel/rcu/rcutorture.c | 67 ++++++++++++++++++++++++++++++++++++++++-
> >  kernel/rcu/tree.c       | 34 +++++++++++++++++++--
> >  kernel/rcu/tree.h       |  1 +
> >  4 files changed, 103 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > index eed2951a4962..516b26024a37 100644
> > --- a/kernel/rcu/rcu.h
> > +++ b/kernel/rcu/rcu.h
> > @@ -572,6 +572,8 @@ void do_trace_rcu_torture_read(const char *rcutorturename,
> >  			       unsigned long c_old,
> >  			       unsigned long c);
> >  void rcu_gp_set_torture_wait(int duration);
> > +void rcu_set_gpwrap_lag(unsigned long lag);
> > +int rcu_get_gpwrap_count(int cpu);
> >  #else
> >  static inline void rcutorture_get_gp_data(int *flags, unsigned long *gp_seq)
> >  {
> > @@ -589,6 +591,8 @@ void do_trace_rcu_torture_read(const char *rcutorturename,
> >  	do { } while (0)
> >  #endif
> >  static inline void rcu_gp_set_torture_wait(int duration) { }
> > +static inline void rcu_set_gpwrap_lag(unsigned long lag) { }
> > +static inline int rcu_get_gpwrap_count(int cpu) { return 0; }
> >  #endif
> >  unsigned long long rcutorture_gather_gp_seqs(void);
> >  void rcutorture_format_gp_seqs(unsigned long long seqs, char *cp, size_t len);
> > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > index 4fa7772be183..74de92c3a9ab 100644
> > --- a/kernel/rcu/rcutorture.c
> > +++ b/kernel/rcu/rcutorture.c
> > @@ -115,6 +115,9 @@ torture_param(int, nreaders, -1, "Number of RCU reader threads");
> >  torture_param(int, object_debug, 0, "Enable debug-object double call_rcu() testing");
> >  torture_param(int, onoff_holdoff, 0, "Time after boot before CPU hotplugs (s)");
> >  torture_param(int, onoff_interval, 0, "Time between CPU hotplugs (jiffies), 0=disable");
> > +torture_param(int, gpwrap_lag_cycle_mins, 30, "Total cycle duration for ovf lag testing (in minutes)");
> > +torture_param(int, gpwrap_lag_active_mins, 5, "Duration for which ovf lag is active within each cycle (in minutes)");
> > +torture_param(int, gpwrap_lag_gps, 8, "Value to set for set_gpwrap_lag during an active testing period.");
> >  torture_param(int, nocbs_nthreads, 0, "Number of NOCB toggle threads, 0 to disable");
> >  torture_param(int, nocbs_toggle, 1000, "Time between toggling nocb state (ms)");
> >  torture_param(int, preempt_duration, 0, "Preemption duration (ms), zero to disable");
> > @@ -413,6 +416,8 @@ struct rcu_torture_ops {
> >  	bool (*reader_blocked)(void);
> >  	unsigned long long (*gather_gp_seqs)(void);
> >  	void (*format_gp_seqs)(unsigned long long seqs, char *cp, size_t len);
> > +	void (*set_gpwrap_lag)(unsigned long lag);
> > +	int (*get_gpwrap_count)(int cpu);
> >  	long cbflood_max;
> >  	int irq_capable;
> >  	int can_boost;
> > @@ -619,6 +624,8 @@ static struct rcu_torture_ops rcu_ops = {
> >  				  : NULL,
> >  	.gather_gp_seqs		= rcutorture_gather_gp_seqs,
> >  	.format_gp_seqs		= rcutorture_format_gp_seqs,
> > +	.set_gpwrap_lag		= rcu_set_gpwrap_lag,
> > +	.get_gpwrap_count	= rcu_get_gpwrap_count,
> >  	.irq_capable		= 1,
> >  	.can_boost		= IS_ENABLED(CONFIG_RCU_BOOST),
> >  	.extendables		= RCUTORTURE_MAX_EXTEND,
> > @@ -2394,6 +2401,7 @@ rcu_torture_stats_print(void)
> >  	int i;
> >  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
> >  	long batchsummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
> > +	long n_gpwraps = 0;
> >  	struct rcu_torture *rtcp;
> >  	static unsigned long rtcv_snap = ULONG_MAX;
> >  	static bool splatted;
> > @@ -2404,6 +2412,7 @@ rcu_torture_stats_print(void)
> >  			pipesummary[i] += READ_ONCE(per_cpu(rcu_torture_count, cpu)[i]);
> >  			batchsummary[i] += READ_ONCE(per_cpu(rcu_torture_batch, cpu)[i]);
> >  		}
> > +		n_gpwraps += cur_ops->get_gpwrap_count(cpu);
> >  	}
> >  	for (i = RCU_TORTURE_PIPE_LEN; i >= 0; i--) {
> >  		if (pipesummary[i] != 0)
> > @@ -2435,8 +2444,9 @@ rcu_torture_stats_print(void)
> >  		data_race(n_barrier_attempts),
> >  		data_race(n_rcu_torture_barrier_error));
> >  	pr_cont("read-exits: %ld ", data_race(n_read_exits)); // Statistic.
> > -	pr_cont("nocb-toggles: %ld:%ld\n",
> > +	pr_cont("nocb-toggles: %ld:%ld ",
> >  		atomic_long_read(&n_nocb_offload), atomic_long_read(&n_nocb_deoffload));
> > +	pr_cont("gpwraps: %ld\n", n_gpwraps);
> >  
> >  	pr_alert("%s%s ", torture_type, TORTURE_FLAG);
> >  	if (atomic_read(&n_rcu_torture_mberror) ||
> > @@ -3607,6 +3617,54 @@ static int rcu_torture_preempt(void *unused)
> >  
> >  static enum cpuhp_state rcutor_hp;
> >  
> > +static struct hrtimer gpwrap_lag_timer;
> > +static bool gpwrap_lag_active;
> > +
> > +/* Timer handler for toggling RCU grace-period sequence overflow test lag value */
> > +static enum hrtimer_restart rcu_gpwrap_lag_timer(struct hrtimer *timer)
> > +{
> > +	ktime_t next_delay;
> > +
> > +	if (gpwrap_lag_active) {
> > +		pr_alert("rcu-torture: Disabling ovf lag (value=0)\n");
> > +		cur_ops->set_gpwrap_lag(0);
> > +		gpwrap_lag_active = false;
> > +		next_delay = ktime_set((gpwrap_lag_cycle_mins - gpwrap_lag_active_mins) * 60, 0);
> > +	} else {
> > +		pr_alert("rcu-torture: Enabling ovf lag (value=%d)\n", gpwrap_lag_gps);
> > +		cur_ops->set_gpwrap_lag(gpwrap_lag_gps);
> > +		gpwrap_lag_active = true;
> > +		next_delay = ktime_set(gpwrap_lag_active_mins * 60, 0);
> > +	}
> > +
> > +	if (torture_must_stop())
> > +		return HRTIMER_NORESTART;
> > +
> > +	hrtimer_forward_now(timer, next_delay);
> > +	return HRTIMER_RESTART;
> > +}
> > +
> > +static int rcu_gpwrap_lag_init(void)
> > +{
> > +	if (gpwrap_lag_cycle_mins <= 0 || gpwrap_lag_active_mins <= 0) {
> > +		pr_alert("rcu-torture: lag timing parameters must be positive\n");
> > +		return -EINVAL;
> 
> When rcutorture is initiated by modprobe, this makes perfect sense.
> 
> But if rcutorture is built in, we have other choices:  (1) Disable gpwrap
> testing and do other testing but splat so that the bogus scripting can
> be fixed, (2) Force default values and splat as before, (3) Splat and
> halt the system.
> 
> The usual approach has been #1, but what makes sense in this case?
> 
> > +	}
> > +
> > +	hrtimer_setup(&gpwrap_lag_timer, rcu_gpwrap_lag_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +	gpwrap_lag_active = false;
> > +	hrtimer_start(&gpwrap_lag_timer,
> > +		      ktime_set((gpwrap_lag_cycle_mins - gpwrap_lag_active_mins) * 60, 0), HRTIMER_MODE_REL);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rcu_gpwrap_lag_cleanup(void)
> > +{
> > +	hrtimer_cancel(&gpwrap_lag_timer);
> > +	cur_ops->set_gpwrap_lag(0);
> > +	gpwrap_lag_active = false;
> > +}
> >  static void
> >  rcu_torture_cleanup(void)
> >  {
> > @@ -3776,6 +3834,9 @@ rcu_torture_cleanup(void)
> >  	torture_cleanup_end();
> >  	if (cur_ops->gp_slow_unregister)
> >  		cur_ops->gp_slow_unregister(NULL);
> > +
> > +	if (cur_ops->set_gpwrap_lag)
> > +		rcu_gpwrap_lag_cleanup();
> >  }
> >  
> >  static void rcu_torture_leak_cb(struct rcu_head *rhp)
> > @@ -4275,6 +4336,10 @@ rcu_torture_init(void)
> >  	torture_init_end();
> >  	if (cur_ops->gp_slow_register && !WARN_ON_ONCE(!cur_ops->gp_slow_unregister))
> >  		cur_ops->gp_slow_register(&rcu_fwd_cb_nodelay);
> > +
> > +	if (cur_ops->set_gpwrap_lag && rcu_gpwrap_lag_init())
> > +		goto unwind;
> > +
> >  	return 0;
> >  
> >  unwind:
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 659f83e71048..6ec30d07759d 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -80,6 +80,15 @@ static void rcu_sr_normal_gp_cleanup_work(struct work_struct *);
> >  static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> >  	.gpwrap = true,
> >  };
> > +
> > +int rcu_get_gpwrap_count(int cpu)
> > +{
> > +	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > +
> > +	return READ_ONCE(rdp->gpwrap_count);
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_get_gpwrap_count);
> > +
> >  static struct rcu_state rcu_state = {
> >  	.level = { &rcu_state.node[0] },
> >  	.gp_state = RCU_GP_IDLE,
> > @@ -757,6 +766,25 @@ void rcu_request_urgent_qs_task(struct task_struct *t)
> >  	smp_store_release(per_cpu_ptr(&rcu_data.rcu_urgent_qs, cpu), true);
> >  }
> >  
> > +/**
> > + * rcu_set_gpwrap_lag - Set RCU GP sequence overflow lag value.
> > + * @lag_gps: Set overflow lag to this many grace period worth of counters
> > + * which is used by rcutorture to quickly force a gpwrap situation.
> > + * @lag_gps = 0 means we reset it back to the boot-time value.
> > + */
> > +static unsigned long seq_gpwrap_lag = ULONG_MAX / 4;
> > +
> > +void rcu_set_gpwrap_lag(unsigned long lag_gps)
> > +{
> > +	unsigned long lag_seq_count;
> > +
> > +	lag_seq_count = (lag_gps == 0)
> > +			? ULONG_MAX / 4
> > +			: lag_gps << RCU_SEQ_CTR_SHIFT;
> > +	WRITE_ONCE(seq_gpwrap_lag, lag_seq_count);
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_set_gpwrap_lag);
> > +
> >  /*
> >   * When trying to report a quiescent state on behalf of some other CPU,
> >   * it is our responsibility to check for and handle potential overflow
> > @@ -767,9 +795,11 @@ void rcu_request_urgent_qs_task(struct task_struct *t)
> >  static void rcu_gpnum_ovf(struct rcu_node *rnp, struct rcu_data *rdp)
> >  {
> >  	raw_lockdep_assert_held_rcu_node(rnp);
> > -	if (ULONG_CMP_LT(rcu_seq_current(&rdp->gp_seq) + ULONG_MAX / 4,
> > -			 rnp->gp_seq))
> > +	if (ULONG_CMP_LT(rcu_seq_current(&rdp->gp_seq) + seq_gpwrap_lag,
> > +			 rnp->gp_seq)) {
> >  		WRITE_ONCE(rdp->gpwrap, true);
> > +		WRITE_ONCE(rdp->gpwrap_count, READ_ONCE(rdp->gpwrap_count) + 1);
> > +	}
> >  	if (ULONG_CMP_LT(rdp->rcu_iw_gp_seq + ULONG_MAX / 4, rnp->gp_seq))
> >  		rdp->rcu_iw_gp_seq = rnp->gp_seq + ULONG_MAX / 4;
> >  }
> > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > index a9a811d9d7a3..63bea388c243 100644
> > --- a/kernel/rcu/tree.h
> > +++ b/kernel/rcu/tree.h
> > @@ -183,6 +183,7 @@ struct rcu_data {
> >  	bool		core_needs_qs;	/* Core waits for quiescent state. */
> >  	bool		beenonline;	/* CPU online at least once. */
> >  	bool		gpwrap;		/* Possible ->gp_seq wrap. */
> > +	unsigned int	gpwrap_count;	/* Count of GP sequence wrap. */
> >  	bool		cpu_started;	/* RCU watching this onlining CPU. */
> >  	struct rcu_node *mynode;	/* This CPU's leaf of hierarchy */
> >  	unsigned long grpmask;		/* Mask to apply to leaf qsmask. */
> > -- 
> > 2.43.0
> > 

Powered by blists - more mailing lists