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
--
2.43.5Le 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)
Let me audit further if we missed something else...
Thanks.
> --
> 2.43.5
>
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);
- if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
+ /*
+ * rcu_report_qs_rdp() can only be invoked with a stable rdp and
+ * and from the local CPU.
+ * With CONFIG_PREEMPTION=y, do this while holding the last
+ * preempt_count which gets dropped after __rcu_read_unlock().
+ */
+ if (irqs_disabled() || pc > 1 || !rcu_state.gp_kthread)
return;
rdp = this_cpu_ptr(&rcu_data);
rdp->cpu_no_qs.b.norm = false;
Thanks
--
ankur
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.
Thanks.
>
> - if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
> + /*
> + * rcu_report_qs_rdp() can only be invoked with a stable rdp and
> + * and from the local CPU.
> + * With CONFIG_PREEMPTION=y, do this while holding the last
> + * preempt_count which gets dropped after __rcu_read_unlock().
> + */
> + if (irqs_disabled() || pc > 1 || !rcu_state.gp_kthread)
> return;
> rdp = this_cpu_ptr(&rcu_data);
> rdp->cpu_no_qs.b.norm = false;
>
>
> Thanks
>
> --
> ankur
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?
And, given that the preempt_count is at least 1, the (pc > 1) check below
would ensure we have a stable rdp and call rcu_report_qs_rdp() before
dropping the last preempt-count.
>>
>> - if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
>> + /*
>> + * rcu_report_qs_rdp() can only be invoked with a stable rdp and
>> + * and from the local CPU.
>> + * With CONFIG_PREEMPTION=y, do this while holding the last
>> + * preempt_count which gets dropped after __rcu_read_unlock().
>> + */
>> + if (irqs_disabled() || pc > 1 || !rcu_state.gp_kthread)
>> return;
>> rdp = this_cpu_ptr(&rcu_data);
>> rdp->cpu_no_qs.b.norm = false;
Thanks
--
ankur
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
Ankur
> And, given that the preempt_count is at least 1, the (pc > 1) check below
> would ensure we have a stable rdp and call rcu_report_qs_rdp() before
> dropping the last preempt-count.
>
>>>
>>> - if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
>>> + /*
>>> + * rcu_report_qs_rdp() can only be invoked with a stable rdp and
>>> + * and from the local CPU.
>>> + * With CONFIG_PREEMPTION=y, do this while holding the last
>>> + * preempt_count which gets dropped after __rcu_read_unlock().
>>> + */
>>> + if (irqs_disabled() || pc > 1 || !rcu_state.gp_kthread)
>>> return;
>>> rdp = this_cpu_ptr(&rcu_data);
>>> rdp->cpu_no_qs.b.norm = false;
>
> Thanks
--
ankur
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.
Frederic Weisbecker <frederic@kernel.org> writes:
> 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?
Sure. I can add it to v3. Okay, if I add your suggested-by?
--
ankur
Le Thu, Nov 28, 2024 at 08:39:01PM -0800, Ankur Arora a écrit : > > Do you plan to integrate this in a further version of your set? Or should I send > > a patch? > > Sure. I can add it to v3. Okay, if I add your suggested-by? Sure! Thanks.
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>
Thanks!
> 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
I think I see your point about rdp being racy, but given that
rcu_read_unlock_strict() would always be called with a non-zero
preemption count (with CONFIG_PREEMPTION), wouldn't the preempt_count()
check defeat any calls to rcu_read_unlock_strict()?
void rcu_read_unlock_strict(void)
{
struct rcu_data *rdp;
if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
return;
Or am I missing something?
Ankur
> (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)
>
>
> Let me audit further if we missed something else...
>
> Thanks.
>
>> --
>> 2.43.5
>>
On 2024-11-13 16:23:03 [-0800], Ankur Arora wrote:
> > 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
>
> I think I see your point about rdp being racy, but given that
> rcu_read_unlock_strict() would always be called with a non-zero
> preemption count (with CONFIG_PREEMPTION), wouldn't the preempt_count()
> check defeat any calls to rcu_read_unlock_strict()?
>
> void rcu_read_unlock_strict(void)
> {
> struct rcu_data *rdp;
>
> if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
> return;
>
> Or am I missing something?
This is indeed broken. By moving preempt_disable() as Frederic suggested
then rcu_read_unlock_strict() becomes a NOP. Keeping this as-is results
in spats due to this_cpu_ptr() in preemptible regions. Looking further
we have "rdp->cpu != smp_processor_id()" as the next candidate.
That preempt_disable() should go to rcu_read_unlock_strict() after the
check.
> Ankur
Sebastian
Le Thu, Nov 14, 2024 at 09:25:34AM +0100, Sebastian Andrzej Siewior a écrit :
> On 2024-11-13 16:23:03 [-0800], Ankur Arora wrote:
> > > 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
> >
> > I think I see your point about rdp being racy, but given that
> > rcu_read_unlock_strict() would always be called with a non-zero
> > preemption count (with CONFIG_PREEMPTION), wouldn't the preempt_count()
> > check defeat any calls to rcu_read_unlock_strict()?
> >
> > void rcu_read_unlock_strict(void)
> > {
> > struct rcu_data *rdp;
> >
> > if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
> > return;
> >
> > Or am I missing something?
Haha, right!
>
> This is indeed broken. By moving preempt_disable() as Frederic suggested
> then rcu_read_unlock_strict() becomes a NOP. Keeping this as-is results
> in spats due to this_cpu_ptr() in preemptible regions. Looking further
> we have "rdp->cpu != smp_processor_id()" as the next candidate.
>
> That preempt_disable() should go to rcu_read_unlock_strict() after the
> check.
Yeah that looks better ;-)
>
> > Ankur
>
> Sebastian
© 2016 - 2026 Red Hat, Inc.