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@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. > 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@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 > > > >
