[PATCH] srcu: Add notes for not-effective mutex

[PATCH] srcu: Add notes for not-effective mutex

[Pingfan Liu]

Capped by the design and implementation of workqueue, the same work can not
be re-entrant.

SRCU has only a single work ssp->work, there is no way to occur the
concurrent scene in its state machine. Hence the mutexes for the
concurrency in the state machine is unnecessary. On the other hand,
using two works to utilize the window from the release of srcu_gp_mutex
to the end of srcu_gp_end() to enhance the parallelism has limited
effect. [1]

To save any trying of improving parallelism by using two work_struct,
put some notes around the srcu_gp_mutex and srcu_cb_mutex. And keep them
instead of removing them, in case in future, the state machine is moved
out of the protection of workqueue non-reentrant protection.

Some test data to show how dual work_struct affects the SRCU
performance:

-1. on 144 cpus hpe-dl560gen10
  modprobe rcutorture   torture_type=srcud  fwd_progress=144 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
  # base line
  [37587.632155] srcud:  End-test grace-period state: g28287244 f0x0 total-gps=28287244
  # two dual work_struct
  [36443.468017] srcud:  End-test grace-period state: g29026056 f0x0 total-gps=29026056

-2. on 256 cpus amd-milan
  modprobe rcutorture   torture_type=srcud  fwd_progress=256 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
  # base line
  [36093.605732] srcud:  End-test grace-period state: g10850284 f0x0 total-gps=10850284
  # two dual work_struct
  [36093.856713] srcud:  End-test grace-period state: g10672632 f0x0 total-gps=10672632

The first test shows that it has about 2.6% improvement, while the
second test shows it has no significant effect.

[1]: https://lore.kernel.org/rcu/Y5nC2yBhaUYirByo@piliu.users.ipa.redhat.com/T/#maf99cf1d92077a1b0b8e5c411c79fa4444576624

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
To: rcu@vger.kernel.org
---
 include/linux/srcutree.h | 4 ++++
 kernel/rcu/srcutree.c    | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 558057b517b7..f199c31493ab 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -66,8 +66,12 @@ struct srcu_struct {
 						/* First node at each level. */
 	int srcu_size_state;			/* Small-to-big transition state. */
 	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
+						/* Not take effect since the only single sdp->work */
+						/* meets the workqueue non-reentrant condition */
 	spinlock_t __private lock;		/* Protect counters and size state. */
 	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
+						/* Not take effect since the only single sdp->work */
+						/* meets the workqueue non-reentrant condition */
 	unsigned int srcu_idx;			/* Current rdr array element. */
 	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
 	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index ce39907fa381..8ebb1a240219 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -815,7 +815,10 @@ static void srcu_gp_end(struct srcu_struct *ssp)
 	struct srcu_node *snp;
 	int ss_state;
 
-	/* Prevent more than one additional grace period. */
+	/*
+	 * Prevent more than one additional grace period. But at present, it does
+	 * not take effect. Refer to the note at the definition.
+	 * /
 	mutex_lock(&ssp->srcu_cb_mutex);
 
 	/* End the current grace period. */
-- 
2.31.1

Re: [PATCH] srcu: Add notes for not-effective mutex

[Paul E. McKenney]

On Wed, Dec 14, 2022 at 10:24:46PM +0800, Pingfan Liu wrote:
> Capped by the design and implementation of workqueue, the same work can not
> be re-entrant.
> 
> SRCU has only a single work ssp->work, there is no way to occur the
> concurrent scene in its state machine. Hence the mutexes for the
> concurrency in the state machine is unnecessary. On the other hand,
> using two works to utilize the window from the release of srcu_gp_mutex
> to the end of srcu_gp_end() to enhance the parallelism has limited
> effect. [1]
> 
> To save any trying of improving parallelism by using two work_struct,
> put some notes around the srcu_gp_mutex and srcu_cb_mutex. And keep them
> instead of removing them, in case in future, the state machine is moved
> out of the protection of workqueue non-reentrant protection.

Thank you for continuning to push on this!

But let's step back and take a wider view, plus summarize a bit.

First, as we have discussed, it is necessary to make RCU work not just
right now, but also to keep it from becoming too fragile.  This notion of
"fragile" isn't yet something that can be reasonably measured, but one
could unreasonably measure it by either comparing the number of words
of English required to explain why it works on the one hand and making
lots of random changes and seeing what fraction of those changes break
something on the other.  These unreasonable measures are, just like the
name says, unreasonable.  But I hope that they give an intuitive notion
of what the goal must be.  Again, I don't know of a reasonable scientific
(let alone mathematical) way of expressing it.

As you noted, the current state is unsatisfying.  There are two different
mechanisms providing mutual exclusion.  This code is nowhere near a
fastpath, so the added overhead is absolutely *not* a concern (not yet,
that we know of, anyway), but there are still risks, for example:

1.	Latent bugs could be introduced that suddenly make their presence
	known should the code ever be relying solely on the mutexes for
	mutual exclusion.

2.	People imagine other purposes for either the single workqueue
	or the mutexes, and inject bugs based on the fact that their
	imagination of the design has diverged from the actual design.

These risks are not huge, especially compared to other risks in the
code base.  For but one example, the code that provides the memory-order
guarantees for synchronize_rcu() are intricate and spread widely over the
code.  As they must be in order to make important fastpaths fast, which
is a benefit that is important enough to make taking that risk worthwhile.

So what are the options here?

1.	Leave it as it is, with the non-huge risks as called out above.

2.	Add comments, as in this commit.  One risk here is that someone
	adds the concurrency, but fails to remove the comments, which
	are now obsolete and confusing.

3.	Add words describing the current double-provided mutual exclusion
	to one of the RCU design documents.

4.	Leave mainline and -rcu as they are, but add your
	multiple-workqueue patch to one of the RCU design documents
	covering SRCU so that it doesn't get lost should it ever become
	necessary to speed up SRCU grace periods.

5.	Leave mainline as it is, but add your multiple-workqueue patch to
	-rcu with a not-for-mainline branch marking its presence so that
	this change can be easily applied later, if needed.  (For another
	example of this, see the sysidle.2017.05.11a branch, which was
	actually in mainline for some time and then later removed because
	it was not being used.)

6.	Make the workqueue supply concurrency, as in your earlier patch.
	As noted in the email threads, this adds risk for no known
	user-visible benefit.

7.	Remove the mutexes, and rely on the workqueue for the needed
	mutual exclusion.  This is unlikely to make the code more
	readable, and makes it more difficult to add concurrency if it
	is ever needed.

8.	Make the workqueue supply concurrency, but only when a given
	kernel-boot parameter or Kconfig option is supplied.  Enable
	this concurrency only during selected rcutorture tests.
	This has the advantage of actually testing the placement and
	usage of the mutexes, but on the other hand it increases test
	effort for no known end-user-visible benefit.

Are there other options?

							Thanx, Paul

> Some test data to show how dual work_struct affects the SRCU
> performance:
> 
> -1. on 144 cpus hpe-dl560gen10
>   modprobe rcutorture   torture_type=srcud  fwd_progress=144 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
>   # base line
>   [37587.632155] srcud:  End-test grace-period state: g28287244 f0x0 total-gps=28287244
>   # two dual work_struct
>   [36443.468017] srcud:  End-test grace-period state: g29026056 f0x0 total-gps=29026056
> 
> -2. on 256 cpus amd-milan
>   modprobe rcutorture   torture_type=srcud  fwd_progress=256 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
>   # base line
>   [36093.605732] srcud:  End-test grace-period state: g10850284 f0x0 total-gps=10850284
>   # two dual work_struct
>   [36093.856713] srcud:  End-test grace-period state: g10672632 f0x0 total-gps=10672632
> 
> The first test shows that it has about 2.6% improvement, while the
> second test shows it has no significant effect.
> 
> [1]: https://lore.kernel.org/rcu/Y5nC2yBhaUYirByo@piliu.users.ipa.redhat.com/T/#maf99cf1d92077a1b0b8e5c411c79fa4444576624
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> To: rcu@vger.kernel.org
> ---
>  include/linux/srcutree.h | 4 ++++
>  kernel/rcu/srcutree.c    | 5 ++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 558057b517b7..f199c31493ab 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -66,8 +66,12 @@ struct srcu_struct {
>  						/* First node at each level. */
>  	int srcu_size_state;			/* Small-to-big transition state. */
>  	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
> +						/* Not take effect since the only single sdp->work */
> +						/* meets the workqueue non-reentrant condition */
>  	spinlock_t __private lock;		/* Protect counters and size state. */
>  	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
> +						/* Not take effect since the only single sdp->work */
> +						/* meets the workqueue non-reentrant condition */
>  	unsigned int srcu_idx;			/* Current rdr array element. */
>  	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
>  	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index ce39907fa381..8ebb1a240219 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -815,7 +815,10 @@ static void srcu_gp_end(struct srcu_struct *ssp)
>  	struct srcu_node *snp;
>  	int ss_state;
>  
> -	/* Prevent more than one additional grace period. */
> +	/*
> +	 * Prevent more than one additional grace period. But at present, it does
> +	 * not take effect. Refer to the note at the definition.
> +	 * /
>  	mutex_lock(&ssp->srcu_cb_mutex);
>  
>  	/* End the current grace period. */
> -- 
> 2.31.1
> 

Re: [PATCH] srcu: Add notes for not-effective mutex

[Pingfan Liu]

On Sun, Dec 18, 2022 at 09:53:13AM -0800, Paul E. McKenney wrote:
> On Wed, Dec 14, 2022 at 10:24:46PM +0800, Pingfan Liu wrote:
> > Capped by the design and implementation of workqueue, the same work can not
> > be re-entrant.
> > 
> > SRCU has only a single work ssp->work, there is no way to occur the
> > concurrent scene in its state machine. Hence the mutexes for the
> > concurrency in the state machine is unnecessary. On the other hand,
> > using two works to utilize the window from the release of srcu_gp_mutex
> > to the end of srcu_gp_end() to enhance the parallelism has limited
> > effect. [1]
> > 
> > To save any trying of improving parallelism by using two work_struct,
> > put some notes around the srcu_gp_mutex and srcu_cb_mutex. And keep them
> > instead of removing them, in case in future, the state machine is moved
> > out of the protection of workqueue non-reentrant protection.
> 
> Thank you for continuning to push on this!
> 
> But let's step back and take a wider view, plus summarize a bit.
> 
> First, as we have discussed, it is necessary to make RCU work not just
> right now, but also to keep it from becoming too fragile.  This notion of
> "fragile" isn't yet something that can be reasonably measured, but one
> could unreasonably measure it by either comparing the number of words
> of English required to explain why it works on the one hand and making
> lots of random changes and seeing what fraction of those changes break
> something on the other.  These unreasonable measures are, just like the
> name says, unreasonable.  But I hope that they give an intuitive notion
> of what the goal must be.  Again, I don't know of a reasonable scientific
> (let alone mathematical) way of expressing it.
> 

Yes, I get the point of srcu_gp_mutex, which keeps the code from
becoming too fragile. But the carefully designed srcu_cb_mutex may leave
room for people to have fantasy. My purpose just reminders someone in
future it is known and tried.

> As you noted, the current state is unsatisfying.  There are two different
> mechanisms providing mutual exclusion.  This code is nowhere near a
> fastpath, so the added overhead is absolutely *not* a concern (not yet,
> that we know of, anyway), but there are still risks, for example:
> 
> 1.	Latent bugs could be introduced that suddenly make their presence
> 	known should the code ever be relying solely on the mutexes for
> 	mutual exclusion.
> 
> 2.	People imagine other purposes for either the single workqueue
> 	or the mutexes, and inject bugs based on the fact that their
> 	imagination of the design has diverged from the actual design.
> 
> These risks are not huge, especially compared to other risks in the
> code base.  For but one example, the code that provides the memory-order
> guarantees for synchronize_rcu() are intricate and spread widely over the
> code.  As they must be in order to make important fastpaths fast, which
> is a benefit that is important enough to make taking that risk worthwhile.
> 

As said above, I understood this and srcu_gp_mutex is not my target. (I
am sorry that my comment may give wrong impression)

> So what are the options here?
> 
> 1.	Leave it as it is, with the non-huge risks as called out above.
> 

Due to the test data, it is dim for the outlook of this patch.

I am fine with option one. But if necessary, I am also OK with some
comment around srcu_cb_mutex, which may save someone's time in future.


Thanks,

	Pingfan

> 2.	Add comments, as in this commit.  One risk here is that someone
> 	adds the concurrency, but fails to remove the comments, which
> 	are now obsolete and confusing.
> 
> 3.	Add words describing the current double-provided mutual exclusion
> 	to one of the RCU design documents.
> 
> 4.	Leave mainline and -rcu as they are, but add your
> 	multiple-workqueue patch to one of the RCU design documents
> 	covering SRCU so that it doesn't get lost should it ever become
> 	necessary to speed up SRCU grace periods.
> 
> 5.	Leave mainline as it is, but add your multiple-workqueue patch to
> 	-rcu with a not-for-mainline branch marking its presence so that
> 	this change can be easily applied later, if needed.  (For another
> 	example of this, see the sysidle.2017.05.11a branch, which was
> 	actually in mainline for some time and then later removed because
> 	it was not being used.)
> 
> 6.	Make the workqueue supply concurrency, as in your earlier patch.
> 	As noted in the email threads, this adds risk for no known
> 	user-visible benefit.
> 
> 7.	Remove the mutexes, and rely on the workqueue for the needed
> 	mutual exclusion.  This is unlikely to make the code more
> 	readable, and makes it more difficult to add concurrency if it
> 	is ever needed.
> 
> 8.	Make the workqueue supply concurrency, but only when a given
> 	kernel-boot parameter or Kconfig option is supplied.  Enable
> 	this concurrency only during selected rcutorture tests.
> 	This has the advantage of actually testing the placement and
> 	usage of the mutexes, but on the other hand it increases test
> 	effort for no known end-user-visible benefit.
> 
> Are there other options?
> 
> 							Thanx, Paul
> 
> > Some test data to show how dual work_struct affects the SRCU
> > performance:
> > 
> > -1. on 144 cpus hpe-dl560gen10
> >   modprobe rcutorture   torture_type=srcud  fwd_progress=144 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
> >   # base line
> >   [37587.632155] srcud:  End-test grace-period state: g28287244 f0x0 total-gps=28287244
> >   # two dual work_struct
> >   [36443.468017] srcud:  End-test grace-period state: g29026056 f0x0 total-gps=29026056
> > 
> > -2. on 256 cpus amd-milan
> >   modprobe rcutorture   torture_type=srcud  fwd_progress=256 fwd_progress_holdoff=1 shutdown_secs=36000 stat_interval=60 verbose=1
> >   # base line
> >   [36093.605732] srcud:  End-test grace-period state: g10850284 f0x0 total-gps=10850284
> >   # two dual work_struct
> >   [36093.856713] srcud:  End-test grace-period state: g10672632 f0x0 total-gps=10672632
> > 
> > The first test shows that it has about 2.6% improvement, while the
> > second test shows it has no significant effect.
> > 
> > [1]: https://lore.kernel.org/rcu/Y5nC2yBhaUYirByo@piliu.users.ipa.redhat.com/T/#maf99cf1d92077a1b0b8e5c411c79fa4444576624
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: "Zhang, Qiang1" <qiang1.zhang@intel.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > To: rcu@vger.kernel.org
> > ---
> >  include/linux/srcutree.h | 4 ++++
> >  kernel/rcu/srcutree.c    | 5 ++++-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index 558057b517b7..f199c31493ab 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -66,8 +66,12 @@ struct srcu_struct {
> >  						/* First node at each level. */
> >  	int srcu_size_state;			/* Small-to-big transition state. */
> >  	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
> > +						/* Not take effect since the only single sdp->work */
> > +						/* meets the workqueue non-reentrant condition */
> >  	spinlock_t __private lock;		/* Protect counters and size state. */
> >  	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
> > +						/* Not take effect since the only single sdp->work */
> > +						/* meets the workqueue non-reentrant condition */
> >  	unsigned int srcu_idx;			/* Current rdr array element. */
> >  	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
> >  	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index ce39907fa381..8ebb1a240219 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -815,7 +815,10 @@ static void srcu_gp_end(struct srcu_struct *ssp)
> >  	struct srcu_node *snp;
> >  	int ss_state;
> >  
> > -	/* Prevent more than one additional grace period. */
> > +	/*
> > +	 * Prevent more than one additional grace period. But at present, it does
> > +	 * not take effect. Refer to the note at the definition.
> > +	 * /
> >  	mutex_lock(&ssp->srcu_cb_mutex);
> >  
> >  	/* End the current grace period. */
> > -- 
> > 2.31.1
> >