Re: [PATCH v4 3/8] rcu: clear defer_qs_pending in handler for compounded sections

On 7/15/2026 4:50 PM, Paul E. McKenney wrote:
> On Thu, Jun 25, 2026 at 08:42:56PM -0400, Joel Fernandes wrote:
>> The deferred-QS irq-work handler previously cleared defer_qs_pending
>> only when the handler ran inside an active rcu_read_lock() critical
>> section (rcu_preempt_depth() > 0).  Paul McKenney pointed out a common
>> multi-segment compound pattern where the handler fires between
>> segments and segment N+1's arming attempt is silently suppressed by
>> the rcu_read_unlock_special() pending-gate:
>>
>>     rcu_read_lock();           // segment 1 starts
>>     // may be preempted/boosted here
>>     local_irq_disable();
>>     rcu_read_unlock();          // segment 1 ends; arms defer_qs_pending
>>     preempt_disable();
>>     local_irq_enable();         // handler MAY fire here: depth==0, but
>>                                 // but preempt is disabled, so it cant
>>                                 // nudge.
>>
>>     rcu_read_lock();            // segment 2 starts
>>     preempt_enable();
>>     local_irq_disable();
>>     rcu_read_unlock();     // arming attempt suppressed incorrectly -- (1)
>>     local_irq_enable();
>>
>> Waiting for the next __note_gp_changes() clear is too slow for the
>> compound case, we need the deferred QS report sooner.
>>
>> Therefore, make the irq_work handler clear defer_qs_pending whenever
>> rcu_in_compounded_section() is true so that (1) can do the arming.
>>
>> In addition, introduce rcu_preempt_deferred_qs_try_report(), a small
>> helper that reports the deferred QS (and releases any RCU priority
>> boost) directly, but only from a clean, non-reader/compound context.
>> When the handler lands in such a clean context it now reports the QS
>> directly instead of merely nudging the scheduler: this makes the
>> irq_work robust under preempt=none / voluntary, where a
>> set_need_resched() nudge would not enter __schedule() at IRQ exit and
>> the QS would otherwise wait for the next tick.  When still compounded,
>> the handler falls back to clearing defer_qs_pending as before.  The
>> bounded-delay rescue hrtimer added in a later patch reuses this same
>> helper.
> 
> The patch generally looks like an unambiguous improvement, but just to
> make sure that I understand...
> 
> None of the code below is even built if CONFIG_PREEMPT_NONE=y.  So is
> the text above referring to kernels built with CONFIG_PREEMPT_DYNAMIC=y
> and booted with either preempt=none or preempt=voluntary?  If so, we
> need to explicitly state that.

I mentioned "preempt=none/voluntary" above, but you want me to also mention
CONFIG_PREEMPT_DYNAMIC=y in the commit message? If yes, I can add that.
However, just to note, this patch is relevant also for fully-preemptible
modes. The preempt=none side-effect of this patch is just a bonus (since we
can't get the aid of the scheduler in those modes).

Let me know if that makes sense, alternatively I can also just remove the
paragraph in the commit messages starting with "In addition,..".

Thanks.


> 
> 							Thanx, Paul
> 
>> Signed-off-by: Joel Fernandes <[email protected]>
>> ---
>>  kernel/rcu/tree_plugin.h | 46 ++++++++++++++++++++++++++++------------
>>  1 file changed, 33 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>> index 7555022bf38a..7768a40677a4 100644
>> --- a/kernel/rcu/tree_plugin.h
>> +++ b/kernel/rcu/tree_plugin.h
>> @@ -702,7 +702,32 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
>>  }
>>  
>>  /*
>> - * Minimal handler to give the scheduler a chance to re-evaluate.
>> + * Report a deferred quiescent state but only from a safe context.
>> + *
>> + * Both callers (the irq_work handler and the bounded-delay rescue hrtimer)
>> + * run in hardirq context, so preempt_count() always has the HARDIRQ bit set;
>> + * the compound-section check below deliberately inspects only the
>> + * PREEMPT_MASK | SOFTIRQ_MASK bits, which reflect the INTERRUPTED caller's
>> + * state, not ours.
>> + */
>> +static bool rcu_preempt_deferred_qs_try_report(struct task_struct *t)
>> +{
>> +	unsigned long flags;
>> +
>> +	if (rcu_preempt_depth() > 0 ||
>> +	    (preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK)))
>> +		return false;
>> +
>> +	if (rcu_preempt_need_deferred_qs(t)) {
>> +		local_irq_save(flags);
>> +		rcu_preempt_deferred_qs_irqrestore(t, flags);
>> +	}
>> +	return true;
>> +}
>> +
>> +/*
>> + * Minimal handler to give the scheduler a chance to re-evaluate, and to
>> + * report the deferred QS directly when the handler lands in a clean context.
>>   */
>>  static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>>  {
>> @@ -712,19 +737,14 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>>  	rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
>>  
>>  	/*
>> -	 * If the IRQ work handler happens to run in the middle of RCU read-side
>> -	 * critical section, it could be ineffective in getting the scheduler's
>> -	 * attention to report a deferred quiescent state (the whole point of the
>> -	 * IRQ work). For this reason, requeue the IRQ work.
>> -	 *
>> -	 * Basically, we want to avoid following situation:
>> -	 * 1. rcu_read_unlock() queues IRQ work (state -> DEFER_QS_PENDING)
>> -	 * 2. CPU enters new rcu_read_lock()
>> -	 * 3. IRQ work runs but cannot report QS due to rcu_preempt_depth() > 0
>> -	 * 4. rcu_read_unlock() does not re-queue work (state still PENDING)
>> -	 * 5. Deferred QS reporting does not happen.
>> +	 * If the handler fired in a clean context, report the deferred QS
>> +	 * directly.  This makes the irq_work robust under preempt=none /
>> +	 * voluntary, where the set_need_resched() nudge would not enter
>> +	 * __schedule() at IRQ exit.  Otherwise we are still inside a reader /
>> +	 * compound section: just clear defer_qs_pending so the next
>> +	 * rcu_read_unlock() can rearm.
>>  	 */
>> -	if (rcu_preempt_depth() > 0)
>> +	if (!rcu_preempt_deferred_qs_try_report(current))
>>  		rcu_defer_qs_clear(rdp);
>>  }
>>  
>> -- 
>> 2.34.1
>>