Re: [PATCH v2 3/6] rcu: limit PREEMPT_RCU configurations

[Frederic Weisbecker <frederic@kernel.org>]

Le Tue, Nov 26, 2024 at 10:19:05PM -0800, Ankur Arora a écrit :
> 
> Ankur Arora <ankur.a.arora@oracle.com> writes:
> 
> > Frederic Weisbecker <frederic@kernel.org> writes:
> >
> >> Le Mon, Nov 25, 2024 at 01:40:39PM -0800, Ankur Arora a écrit :
> >>>
> >>> Frederic Weisbecker <frederic@kernel.org> writes:
> >>>
> >>> > Le Wed, Nov 06, 2024 at 12:17:55PM -0800, Ankur Arora a écrit :
> >>> >> PREEMPT_LAZY can be enabled stand-alone or alongside PREEMPT_DYNAMIC
> >>> >> which allows for dynamic switching of preemption models.
> >>> >>
> >>> >> The choice of PREEMPT_RCU or not, however, is fixed at compile time.
> >>> >>
> >>> >> Given that PREEMPT_RCU makes some trade-offs to optimize for latency
> >>> >> as opposed to throughput, configurations with limited preemption
> >>> >> might prefer the stronger forward-progress guarantees of PREEMPT_RCU=n.
> >>> >>
> >>> >> Accordingly, explicitly limit PREEMPT_RCU=y to the latency oriented
> >>> >> preemption models: PREEMPT, PREEMPT_RT, and the runtime configurable
> >>> >> model PREEMPT_DYNAMIC.
> >>> >>
> >>> >> This means the throughput oriented models, PREEMPT_NONE,
> >>> >> PREEMPT_VOLUNTARY and PREEMPT_LAZY will run with PREEMPT_RCU=n.
> >>> >>
> >>> >> Cc: Paul E. McKenney <paulmck@kernel.org>
> >>> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >>> >> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> >>> >> ---
> >>> >>  kernel/rcu/Kconfig | 2 +-
> >>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>> >>
> >>> >> diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> >>> >> index 5a7ff5e1cdcb..9d52f87fac27 100644
> >>> >> --- a/kernel/rcu/Kconfig
> >>> >> +++ b/kernel/rcu/Kconfig
> >>> >> @@ -18,7 +18,7 @@ config TREE_RCU
> >>> >>
> >>> >>  config PREEMPT_RCU
> >>> >>  	bool
> >>> >> -	default y if PREEMPTION
> >>> >> +	default y if (PREEMPT || PREEMPT_RT || PREEMPT_DYNAMIC)
> >>> >>  	select TREE_RCU
> >>> >>  	help
> >>> >>  	  This option selects the RCU implementation that is
> >>> >
> >>> > Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> >>> >
> >>> > But looking at !CONFIG_PREEMPT_RCU code on tree_plugin.h, I see
> >>> > some issues now that the code can be preemptible. Well I think
> >>> > it has always been preemptible but PREEMPTION && !PREEMPT_RCU
> >>> > has seldom been exerciced (or was it even possible?).
> >>> >
> >>> > For example rcu_read_unlock_strict() can be called with preemption
> >>> > enabled so we need the following otherwise the rdp is unstable, the
> >>> > norm value becomes racy (though automagically fixed in rcu_report_qs_rdp())
> >>> > and rcu_report_qs_rdp() might warn.
> >>> >
> >>> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> >>> > index 58d84c59f3dd..368f00267d4e 100644
> >>> > --- a/include/linux/rcupdate.h
> >>> > +++ b/include/linux/rcupdate.h
> >>> > @@ -95,9 +95,9 @@ static inline void __rcu_read_lock(void)
> >>> >
> >>> >  static inline void __rcu_read_unlock(void)
> >>> >  {
> >>> > -	preempt_enable();
> >>> >  	if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
> >>> >  		rcu_read_unlock_strict();
> >>> > +	preempt_enable();
> >>> >  }
> >>> >
> >>> >  static inline int rcu_preempt_depth(void)
> >>>
> >>> Based on the discussion on the thread, how about keeping this and
> >>> changing the preempt_count check in rcu_read_unlock_strict() instead?
> >>>
> >>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> >>> index 1c7cbd145d5e..8fc67639d3a7 100644
> >>> --- a/kernel/rcu/tree_plugin.h
> >>> +++ b/kernel/rcu/tree_plugin.h
> >>> @@ -831,8 +831,15 @@ dump_blkd_tasks(struct rcu_node *rnp, int ncheck)
> >>>  void rcu_read_unlock_strict(void)
> >>>  {
> >>>         struct rcu_data *rdp;
> >>> +       int pc = ((preempt_count() & PREEMPT_MASK) >> PREEMPT_SHIFT);
> >>
> >> This should be in_atomic_preempt_off(), otherwise softirqs and IRQs are
> >> spuriously accounted as quiescent states.
> >
> > Not sure I got that. Won't ((preempt_count() & PREEMPT_MASK) >> PREEMPT_SHIFT)
> > give us task only preempt count?
> 
> Oh wait. I see your point now. My check is too narrow.
> 
> Great. This'll work:
> 
> -       if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
> +       if (irqs_disabled() || in_atomic_preempt_off()|| !rcu_state.gp_kthread)
> 
> Thanks

Do you plan to integrate this in a further version of your set? Or should I send
a patch?

Thanks.