linux-kernel - Re: [PATCH v2 3/6] rcu: limit PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87serfgffs.fsf@oracle.com>
Date: Mon, 25 Nov 2024 13:40:39 -0800
From: Ankur Arora <ankur.a.arora@...cle.com>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: Ankur Arora <ankur.a.arora@...cle.com>, linux-kernel@...r.kernel.org,
        peterz@...radead.org, tglx@...utronix.de, paulmck@...nel.org,
        mingo@...nel.org, bigeasy@...utronix.de, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        vschneid@...hat.com, efault@....de, sshegde@...ux.ibm.com,
        boris.ostrovsky@...cle.com
Subject: Re: [PATCH v2 3/6] rcu: limit PREEMPT_RCU configurations


Frederic Weisbecker <frederic@...nel.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@...nel.org>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.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@...nel.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