diff mbox series

[PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL

Message ID 20221130033459.16372-1-piliu@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PATCHv2] srcu: Eliminate the requirement of SRCU_SIZE_WAIT_CALL | expand

Commit Message

Pingfan Liu Nov. 30, 2022, 3:34 a.m. UTC
From: Pingfan Liu <kernelfans@gmail.com>

The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
And it is not needed. Because counter_wrap_check has guarantee that both
srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
srcu_gp_seq and no false alarm will be raised by the statement in
srcu_gp_start_if_needed()

  ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)

As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
used immediately, not need to wait for another srcu_gp_end() to update
the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.

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>
To: rcu@vger.kernel.org
---
v1 -> v2:
 rebase onto dev branch
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul E. McKenney Dec. 2, 2022, 10:17 p.m. UTC | #1
On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> From: Pingfan Liu <kernelfans@gmail.com>
> 
> The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> And it is not needed. Because counter_wrap_check has guarantee that both
> srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> srcu_gp_seq and no false alarm will be raised by the statement in
> srcu_gp_start_if_needed()
> 
>   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> 
> As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> used immediately, not need to wait for another srcu_gp_end() to update
> the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.

During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
the boot CPU's callback list, but the srcu_barrier() function scans all
of the CPUs' callback lists.

So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
and rcu_segcblist_accelerate() a callback list that is guaranteed to be
empty?  Without this change, the might-not-be-empty boot CPU's callback
list is dealt with.

What am I missing here?  More specifically, what benefits does this
change provide?  I am not yet seeing any.

							Thanx, Paul

> 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>
> To: rcu@vger.kernel.org
> ---
> v1 -> v2:
>  rebase onto dev branch
>  kernel/rcu/srcutree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index b25f77df9e5e..b0278d1e9c1f 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
>  	 */
>  	idx = __srcu_read_lock_nmisafe(ssp);
>  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
>  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
>  	else
>  		sdp = raw_cpu_ptr(ssp->sda);
> -- 
> 2.31.1
>
Pingfan Liu Dec. 13, 2022, 12:51 p.m. UTC | #2
Sorry to reply late. I was interrupted by something else.

On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > From: Pingfan Liu <kernelfans@gmail.com>
> > 
> > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > And it is not needed. Because counter_wrap_check has guarantee that both
> > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > srcu_gp_seq and no false alarm will be raised by the statement in
> > srcu_gp_start_if_needed()
> > 
> >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > 
> > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > used immediately, not need to wait for another srcu_gp_end() to update
> > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> 
> During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> the boot CPU's callback list, but the srcu_barrier() function scans all
> of the CPUs' callback lists.
> 

So at present, your concern is around how it affects srcu_barrier(), right?

> So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> empty?  Without this change, the might-not-be-empty boot CPU's callback
> list is dealt with.
> 

Is it a problem that srcu_gp_start_if_needed() asks
rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
srcu_barrier() will take care of all cpus' callback list.

> What am I missing here?  More specifically, what benefits does this
> change provide?  I am not yet seeing any.
> 

I am trying to improve the note around SRCU_SIZE_xx, and this is a
trivial improve from 8 SRCU_SIZE state to 5.


Thanks,

	Pingfan

> 							Thanx, Paul
> 
> > 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>
> > To: rcu@vger.kernel.org
> > ---
> > v1 -> v2:
> >  rebase onto dev branch
> >  kernel/rcu/srcutree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index b25f77df9e5e..b0278d1e9c1f 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> >  	 */
> >  	idx = __srcu_read_lock_nmisafe(ssp);
> >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> >  	else
> >  		sdp = raw_cpu_ptr(ssp->sda);
> > -- 
> > 2.31.1
> >
Paul E. McKenney Dec. 13, 2022, 8:02 p.m. UTC | #3
On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> Sorry to reply late. I was interrupted by something else.
> 
> On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > From: Pingfan Liu <kernelfans@gmail.com>
> > > 
> > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > srcu_gp_start_if_needed()
> > > 
> > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > 
> > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > used immediately, not need to wait for another srcu_gp_end() to update
> > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > 
> > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > the boot CPU's callback list, but the srcu_barrier() function scans all
> > of the CPUs' callback lists.
> 
> So at present, your concern is around how it affects srcu_barrier(), right?

At present, as always, I am concerned about all potential added bugs
and potential added complexity, both in terms of lines of code and in
terms of size and irregularity of state space.

And yes, part of the concern about both correctness and complexity is
the interaction between call_srcu() and srcu_barrier().  Right now, the
design very straightforwardly ensures that rcu_barrier() and the rest of
SRCU is looking at all CPU's callback lists before call_srcu() places any
callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
mode must have completed before any call_srcu() invocation uses a non-boot
CPU's callback list.

Your change causes rcu_barrier() and call_srcu() to start using non-boot
CPUs' callback lists at the same time.  This change therefore introduces
significant state-space complexity.  And maybe even bugs, if not today
then some later time when someone makes a change that looks obviously
correct on the surface, but which subtly weakens the ordering.

> > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > list is dealt with.
> 
> Is it a problem that srcu_gp_start_if_needed() asks
> rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> srcu_barrier() will take care of all cpus' callback list.

The other way around, please.  My concern is instead that it *fails*
to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
might-not-be-empty callback lists.

> > What am I missing here?  More specifically, what benefits does this
> > change provide?  I am not yet seeing any.
> 
> I am trying to improve the note around SRCU_SIZE_xx, and this is a
> trivial improve from 8 SRCU_SIZE state to 5.

I do understand that you would like to remove SRCU_SIZE_* states.
What I need from you is why it is important to remove these states.
What is being gained by removing them?  Why is that gain so important
that we should (at the very least) make the state space more complex?
And does this change really avoid introducing bugs?

							Thanx, Paul

> Thanks,
> 
> 	Pingfan
> 
> > 							Thanx, Paul
> > 
> > > 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>
> > > To: rcu@vger.kernel.org
> > > ---
> > > v1 -> v2:
> > >  rebase onto dev branch
> > >  kernel/rcu/srcutree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > >  	 */
> > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > >  	else
> > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > -- 
> > > 2.31.1
> > >
Pingfan Liu Dec. 15, 2022, 3:37 a.m. UTC | #4
On Tue, Dec 13, 2022 at 12:02:07PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> > Sorry to reply late. I was interrupted by something else.
> > 
> > On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > > From: Pingfan Liu <kernelfans@gmail.com>
> > > > 
> > > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > > srcu_gp_start_if_needed()
> > > > 
> > > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > > 
> > > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > > used immediately, not need to wait for another srcu_gp_end() to update
> > > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > > 
> > > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > > the boot CPU's callback list, but the srcu_barrier() function scans all
> > > of the CPUs' callback lists.
> > 
> > So at present, your concern is around how it affects srcu_barrier(), right?
> 
> At present, as always, I am concerned about all potential added bugs
> and potential added complexity, both in terms of lines of code and in
> terms of size and irregularity of state space.
> 
> And yes, part of the concern about both correctness and complexity is
> the interaction between call_srcu() and srcu_barrier().  Right now, the
> design very straightforwardly ensures that rcu_barrier() and the rest of
> SRCU is looking at all CPU's callback lists before call_srcu() places any
> callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
> that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
> mode must have completed before any call_srcu() invocation uses a non-boot
> CPU's callback list.
> 

Catch your point. Thanks for patient explanation. And indeed this does
not rely on anything like memory order to build a total order, and more
reliable.

> Your change causes rcu_barrier() and call_srcu() to start using non-boot
> CPUs' callback lists at the same time.  This change therefore introduces
> significant state-space complexity.  And maybe even bugs, if not today
> then some later time when someone makes a change that looks obviously
> correct on the surface, but which subtly weakens the ordering.
> 

Yes, that is true.

> > > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > > list is dealt with.
> > 
> > Is it a problem that srcu_gp_start_if_needed() asks
> > rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> > not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> > srcu_barrier() will take care of all cpus' callback list.
> 
> The other way around, please.  My concern is instead that it *fails*
> to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
> might-not-be-empty callback lists.
> 
> > > What am I missing here?  More specifically, what benefits does this
> > > change provide?  I am not yet seeing any.
> > 
> > I am trying to improve the note around SRCU_SIZE_xx, and this is a
> > trivial improve from 8 SRCU_SIZE state to 5.
> 
> I do understand that you would like to remove SRCU_SIZE_* states.
> What I need from you is why it is important to remove these states.
> What is being gained by removing them?  Why is that gain so important
> that we should (at the very least) make the state space more complex?
> And does this change really avoid introducing bugs?
> 

I am driven by the curiosity about the size state machine, and try to
document it as Frederic asked [1].

And yes as you demand, it should not introduce any complexity without
gains.

Again, appreciate for your detailed explanation. I have a better
understainding of the design. Later I will try to document it with a
patch.

[1]: https://lore.kernel.org/rcu/20221117142025.GE839309@lothringen/

> 							Thanx, Paul
> 
> > Thanks,
> > 
> > 	Pingfan
> > 
> > > 							Thanx, Paul
> > > 
> > > > 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>
> > > > To: rcu@vger.kernel.org
> > > > ---
> > > > v1 -> v2:
> > > >  rebase onto dev branch
> > > >  kernel/rcu/srcutree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > >  	 */
> > > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > > >  	else
> > > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > > -- 
> > > > 2.31.1
> > > >
Paul E. McKenney Dec. 16, 2022, 4:54 p.m. UTC | #5
On Thu, Dec 15, 2022 at 11:37:41AM +0800, Pingfan Liu wrote:
> On Tue, Dec 13, 2022 at 12:02:07PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 13, 2022 at 08:51:29PM +0800, Pingfan Liu wrote:
> > > Sorry to reply late. I was interrupted by something else.
> > > 
> > > On Fri, Dec 02, 2022 at 02:17:13PM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 30, 2022 at 11:34:59AM +0800, Pingfan Liu wrote:
> > > > > From: Pingfan Liu <kernelfans@gmail.com>
> > > > > 
> > > > > The state SRCU_SIZE_WAIT_CALL is only used in srcu_gp_start_if_needed().
> > > > > And it is not needed. Because counter_wrap_check has guarantee that both
> > > > > srcu_gp_seq_needed and srcu_gp_seq_needed_exp are not far behind
> > > > > srcu_gp_seq and no false alarm will be raised by the statement in
> > > > > srcu_gp_start_if_needed()
> > > > > 
> > > > >   ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)
> > > > > 
> > > > > As a result, once if SRCU_SIZE_WAIT_BARRIER is seen, the tree snp can be
> > > > > used immediately, not need to wait for another srcu_gp_end() to update
> > > > > the srcu_gp_seq_needed and srcu_gp_seq_needed_exp to avoid false alarm.
> > > > 
> > > > During the SRCU_SIZE_WAIT_BARRIER state, all callbacks are queued onto
> > > > the boot CPU's callback list, but the srcu_barrier() function scans all
> > > > of the CPUs' callback lists.
> > > 
> > > So at present, your concern is around how it affects srcu_barrier(), right?
> > 
> > At present, as always, I am concerned about all potential added bugs
> > and potential added complexity, both in terms of lines of code and in
> > terms of size and irregularity of state space.
> > 
> > And yes, part of the concern about both correctness and complexity is
> > the interaction between call_srcu() and srcu_barrier().  Right now, the
> > design very straightforwardly ensures that rcu_barrier() and the rest of
> > SRCU is looking at all CPU's callback lists before call_srcu() places any
> > callbacks on non-boot CPUs.  This is because all the srcu_barrier() calls
> > that think that the system is still in SRCU_SIZE_SMALL or SRCU_SIZE_ALLOC
> > mode must have completed before any call_srcu() invocation uses a non-boot
> > CPU's callback list.
> > 
> 
> Catch your point. Thanks for patient explanation. And indeed this does
> not rely on anything like memory order to build a total order, and more
> reliable.
> 
> > Your change causes rcu_barrier() and call_srcu() to start using non-boot
> > CPUs' callback lists at the same time.  This change therefore introduces
> > significant state-space complexity.  And maybe even bugs, if not today
> > then some later time when someone makes a change that looks obviously
> > correct on the surface, but which subtly weakens the ordering.
> > 
> 
> Yes, that is true.
> 
> > > > So can't this cause srcu_gp_start_if_needed() to rcu_segcblist_advance()
> > > > and rcu_segcblist_accelerate() a callback list that is guaranteed to be
> > > > empty?  Without this change, the might-not-be-empty boot CPU's callback
> > > > list is dealt with.
> > > 
> > > Is it a problem that srcu_gp_start_if_needed() asks
> > > rcu_segcblist_advance() and rcu_segcblist_accelerate() to manipulate an
> > > not-be-empty callback list ? At SRCU_SIZE_WAIT_BARRIER state,
> > > srcu_barrier() will take care of all cpus' callback list.
> > 
> > The other way around, please.  My concern is instead that it *fails*
> > to rcu_segcblist_advance() and rcu_segcblist_accelerate() the
> > might-not-be-empty callback lists.
> > 
> > > > What am I missing here?  More specifically, what benefits does this
> > > > change provide?  I am not yet seeing any.
> > > 
> > > I am trying to improve the note around SRCU_SIZE_xx, and this is a
> > > trivial improve from 8 SRCU_SIZE state to 5.
> > 
> > I do understand that you would like to remove SRCU_SIZE_* states.
> > What I need from you is why it is important to remove these states.
> > What is being gained by removing them?  Why is that gain so important
> > that we should (at the very least) make the state space more complex?
> > And does this change really avoid introducing bugs?
> > 
> 
> I am driven by the curiosity about the size state machine, and try to
> document it as Frederic asked [1].
> 
> And yes as you demand, it should not introduce any complexity without
> gains.
> 
> Again, appreciate for your detailed explanation. I have a better
> understainding of the design. Later I will try to document it with a
> patch.

That sounds good, and I look forward to seeing your state-explanation
patch!

							Thanx, Paul

> [1]: https://lore.kernel.org/rcu/20221117142025.GE839309@lothringen/
> 
> > 							Thanx, Paul
> > 
> > > Thanks,
> > > 
> > > 	Pingfan
> > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > 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>
> > > > > To: rcu@vger.kernel.org
> > > > > ---
> > > > > v1 -> v2:
> > > > >  rebase onto dev branch
> > > > >  kernel/rcu/srcutree.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index b25f77df9e5e..b0278d1e9c1f 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -1165,7 +1165,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > >  	 */
> > > > >  	idx = __srcu_read_lock_nmisafe(ssp);
> > > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > > -	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > > +	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
> > > > >  		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
> > > > >  	else
> > > > >  		sdp = raw_cpu_ptr(ssp->sda);
> > > > > -- 
> > > > > 2.31.1
> > > > >
diff mbox series

Patch

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index b25f77df9e5e..b0278d1e9c1f 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1165,7 +1165,7 @@  static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	 */
 	idx = __srcu_read_lock_nmisafe(ssp);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
-	if (ss_state < SRCU_SIZE_WAIT_CALL)
+	if (ss_state < SRCU_SIZE_WAIT_BARRIER)
 		sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
 	else
 		sdp = raw_cpu_ptr(ssp->sda);