linux-kernel - Re: [PATCH -rcu -next 4/7] rcu: Remove redundant check for irq state during unlock

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ebfa8f35-8ede-46c3-a865-384d12d11e90@nvidia.com>
Date: Fri, 11 Jul 2025 12:30:08 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: paulmck@...nel.org
Cc: linux-kernel@...r.kernel.org, Frederic Weisbecker <frederic@...nel.org>,
 Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
 Josh Triplett <josh@...htriplett.org>, Boqun Feng <boqun.feng@...il.com>,
 Uladzislau Rezki <urezki@...il.com>, Steven Rostedt <rostedt@...dmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Lai Jiangshan <jiangshanlai@...il.com>, Zqiang <qiang.zhang@...ux.dev>,
 rcu@...r.kernel.org
Subject: Re: [PATCH -rcu -next 4/7] rcu: Remove redundant check for irq state
 during unlock



On 7/10/2025 8:00 PM, Paul E. McKenney wrote:
> On Tue, Jul 08, 2025 at 10:22:21AM -0400, Joel Fernandes wrote:
>> The check for irqs_were_disabled is redundant in
>> rcu_unlock_needs_exp_handling() as the caller already checks for this.
>> This includes the boost case as well. Just remove the redundant check.
> 
> But in the very last "if" statement in the context of the last hunk of
> this patch, isn't it instead checking for !irqs_were_disabled?
> 
> Or is there something that I am missing that makes this work out OK?

You are right, after going over all the cases I introduced a behavioral change.

Say, irqs_were_disabled was false. And say we are RCU-boosted. needs_exp might
return true now, but previously it was returning false. Further say, we are not
in hardirq.

New code will raise softirq, old code would go to the ELSE and just set:
                set_tsk_need_resched(current);
set_preempt_need_resched();

But preempt_bh_were_disabled that's why we're here.

So we need to only set resched and not raise softirq, because the preempt enable
would reschedule.

Sorry I missed this, but unless this behavior is correct or needs further work,
lets drop this patch. Thanks and kudos on the catch!

 - Joel

> 
> 							Thanx, Paul
> 
>> This is a first win for the refactor of the needs_exp (formerly
>> expboost) condition into a new rcu_unlock_needs_exp_handling() function,
>> as the conditions became more easier to read.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
>> ---
>>  kernel/rcu/tree_plugin.h | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>> index e20c17163c13..7d03d81e45f6 100644
>> --- a/kernel/rcu/tree_plugin.h
>> +++ b/kernel/rcu/tree_plugin.h
>> @@ -659,14 +659,12 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>>   * @t: The task being checked
>>   * @rdp: The per-CPU RCU data
>>   * @rnp: The RCU node for this CPU
>> - * @irqs_were_disabled: Whether interrupts were disabled before rcu_read_unlock()
>>   *
>>   * Returns true if expedited processing of the rcu_read_unlock() is needed.
>>   */
>>  static bool rcu_unlock_needs_exp_handling(struct task_struct *t,
>>  				      struct rcu_data *rdp,
>> -				      struct rcu_node *rnp,
>> -				      bool irqs_were_disabled)
>> +				      struct rcu_node *rnp)
>>  {
>>  	/*
>>  	 * Check if this task is blocking an expedited grace period. If the
>> @@ -702,14 +700,14 @@ static bool rcu_unlock_needs_exp_handling(struct task_struct *t,
>>  
>>  	/*
>>  	 * RCU priority boosting case: If a task is subject to RCU priority
>> -	 * boosting and exits an RCU read-side critical section with interrupts
>> -	 * disabled, we need expedited handling to ensure timely deboosting.
>> -	 * Without this, a low-priority task could incorrectly run at high
>> -	 * real-time priority for an extended period degrading real-time
>> +	 * boosting and exits an RCU read-side critical section, we need
>> +	 * expedited handling to ensure timely deboosting. Without this,
>> +	 * a low-priority task could incorrectly run at high real-time
>> +	 * priority for an extended period degrading real-time
>>  	 * responsiveness. This applies to all CONFIG_RCU_BOOST=y kernels,
>>  	 * not just to PREEMPT_RT.
>>  	 */
>> -	if (IS_ENABLED(CONFIG_RCU_BOOST) && irqs_were_disabled && t->rcu_blocked_node)
>> +	if (IS_ENABLED(CONFIG_RCU_BOOST) && t->rcu_blocked_node)
>>  		return true;
>>  
>>  	return false;
>> @@ -738,7 +736,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
>>  		struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>>  		struct rcu_node *rnp = rdp->mynode;
>>  
>> -		needs_exp = rcu_unlock_needs_exp_handling(t, rdp, rnp, irqs_were_disabled);
>> +		needs_exp = rcu_unlock_needs_exp_handling(t, rdp, rnp);
>>  	
>>  		// Need to defer quiescent state until everything is enabled.
>>  		if (use_softirq && (in_hardirq() || (needs_exp && !irqs_were_disabled))) {
>> -- 
>> 2.34.1
>>