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@xxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx> > > > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx> > > > Cc: Frederic Weisbecker <frederic@xxxxxxxxxx> > > > Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx> > > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > > > Cc: "Zhang, Qiang1" <qiang1.zhang@xxxxxxxxx> > > > To: rcu@xxxxxxxxxxxxxxx > > > --- > > > 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 > > >
