* [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special()
2025-07-05 20:39 [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
@ 2025-07-05 20:39 ` Joel Fernandes
2025-07-06 17:18 ` Paul E. McKenney
2025-07-05 20:39 ` [PATCH RFC 3/3] rcu: Remove redundant check for irq state during unlock Joel Fernandes
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Joel Fernandes @ 2025-07-05 20:39 UTC (permalink / raw)
To: linux-kernel, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Boqun Feng,
Uladzislau Rezki, Steven Rostedt, Mathieu Desnoyers,
Lai Jiangshan, Zqiang, Sebastian Andrzej Siewior, Clark Williams
Cc: rcu, linux-rt-devel
Extract the complex expedited handling condition in rcu_read_unlock_special()
into a separate function rcu_unlock_needs_exp_handling() with detailed
comments explaining each condition.
This improves code readability. No functional change intended.
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
kernel/rcu/tree_plugin.h | 80 +++++++++++++++++++++++++++++++++++-----
1 file changed, 71 insertions(+), 9 deletions(-)
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index baf57745b42f..8504d95bb35b 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -647,6 +647,72 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
local_irq_restore(flags);
}
+/*
+ * Check if expedited grace period processing during unlock is needed.
+ *
+ * This function determines whether expedited handling is required based on:
+ * 1. Task blocking an expedited grace period
+ * 2. CPU participating in an expedited grace period
+ * 3. Strict grace period mode requiring expedited handling
+ * 4. RCU priority boosting needs when interrupts were disabled
+ *
+ * @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)
+{
+ /*
+ * Check if this task is blocking an expedited grace period.
+ * If the task was preempted within an RCU read-side critical section
+ * and is on the expedited grace period blockers list (exp_tasks),
+ * we need expedited handling to unblock the expedited GP.
+ */
+ if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
+ return true;
+
+ /*
+ * Check if this CPU is participating in an expedited grace period.
+ * The expmask bitmap tracks which CPUs need to check in for the
+ * current expedited GP. If our CPU's bit is set, we need expedited
+ * handling to help complete the expedited GP.
+ */
+ if (rdp->grpmask & READ_ONCE(rnp->expmask))
+ return true;
+
+ /*
+ * In CONFIG_RCU_STRICT_GRACE_PERIOD=y kernels, all grace periods
+ * are treated as short for testing purposes even if that means
+ * disturbing the system more. Check if either:
+ * - This CPU has not yet reported a quiescent state, or
+ * - This task was preempted within an RCU critical section
+ * In either case, requird expedited handling for strict GP mode.
+ */
+ if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
+ ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node))
+ return true;
+
+ /*
+ * 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 effecting real-time
+ * responsiveness. This applies to all CONFIG_RCU_BOOST=y kernels,
+ * not just PREEMPT_RT.
+ */
+ if (IS_ENABLED(CONFIG_RCU_BOOST) && irqs_were_disabled && t->rcu_blocked_node)
+ return true;
+
+ return false;
+}
+
/*
* Handle special cases during rcu_read_unlock(), such as needing to
* notify RCU core processing or task having blocked during the RCU
@@ -666,18 +732,14 @@ static void rcu_read_unlock_special(struct task_struct *t)
local_irq_save(flags);
irqs_were_disabled = irqs_disabled_flags(flags);
if (preempt_bh_were_disabled || irqs_were_disabled) {
- bool expboost; // Expedited GP in flight or possible boosting.
+ bool needs_exp; // Expedited handling needed.
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
struct rcu_node *rnp = rdp->mynode;
- expboost = (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks)) ||
- (rdp->grpmask & READ_ONCE(rnp->expmask)) ||
- (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
- ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node)) ||
- (IS_ENABLED(CONFIG_RCU_BOOST) && irqs_were_disabled &&
- t->rcu_blocked_node);
+ needs_exp = rcu_unlock_needs_exp_handling(t, rdp, rnp, irqs_were_disabled);
+
// Need to defer quiescent state until everything is enabled.
- if (use_softirq && (in_hardirq() || (expboost && !irqs_were_disabled))) {
+ if (use_softirq && (in_hardirq() || (needs_exp && !irqs_were_disabled))) {
// Using softirq, safe to awaken, and either the
// wakeup is free or there is either an expedited
// GP in flight or a potential need to deboost.
@@ -690,7 +752,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
set_tsk_need_resched(current);
set_preempt_need_resched();
if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
- expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
+ needs_exp && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
// Get scheduler to re-evaluate and call hooks.
// If !IRQ_WORK, FQS scan will eventually IPI.
if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special()
2025-07-05 20:39 ` [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special() Joel Fernandes
@ 2025-07-06 17:18 ` Paul E. McKenney
2025-07-06 19:16 ` Joel Fernandes
0 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2025-07-06 17:18 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Sebastian Andrzej Siewior, Clark Williams, rcu, linux-rt-devel
On Sat, Jul 05, 2025 at 04:39:16PM -0400, Joel Fernandes wrote:
> Extract the complex expedited handling condition in rcu_read_unlock_special()
> into a separate function rcu_unlock_needs_exp_handling() with detailed
> comments explaining each condition.
>
> This improves code readability. No functional change intended.
Very nice!!!
Some questions and comments interspersed below.
Thanx, Paul
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> kernel/rcu/tree_plugin.h | 80 +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 71 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index baf57745b42f..8504d95bb35b 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -647,6 +647,72 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> local_irq_restore(flags);
> }
>
> +/*
> + * Check if expedited grace period processing during unlock is needed.
> + *
> + * This function determines whether expedited handling is required based on:
> + * 1. Task blocking an expedited grace period
This is a heuristic. What we are actually checking is whether the task
is blocking *some* grace period and whether at least one task (maybe
this one, maybe not) is blocking an expedited grace period.
Why not an exact check? Because that would mean traversing the list
starting at ->exp_tasks, and that list could potentially contain every
task in the system. And I have received bug reports encountered on
systems with hundreds of thousands of tasks.
I could imagine a more complex data structure that semi-efficiently
tracked exact information, but I could also imagine this not being worth
the effort.
> + * 2. CPU participating in an expedited grace period
> + * 3. Strict grace period mode requiring expedited handling
> + * 4. RCU priority boosting needs when interrupts were disabled
s/boosting/deboosting/
> + *
> + * @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)
> +{
> + /*
> + * Check if this task is blocking an expedited grace period.
> + * If the task was preempted within an RCU read-side critical section
> + * and is on the expedited grace period blockers list (exp_tasks),
> + * we need expedited handling to unblock the expedited GP.
Please see above for the heuristic nature of this check.
> + */
> + if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
> + return true;
> +
> + /*
> + * Check if this CPU is participating in an expedited grace period.
> + * The expmask bitmap tracks which CPUs need to check in for the
> + * current expedited GP. If our CPU's bit is set, we need expedited
> + * handling to help complete the expedited GP.
> + */
> + if (rdp->grpmask & READ_ONCE(rnp->expmask))
> + return true;
> +
> + /*
> + * In CONFIG_RCU_STRICT_GRACE_PERIOD=y kernels, all grace periods
> + * are treated as short for testing purposes even if that means
> + * disturbing the system more. Check if either:
> + * - This CPU has not yet reported a quiescent state, or
> + * - This task was preempted within an RCU critical section
> + * In either case, requird expedited handling for strict GP mode.
s/requird/required/ ;-)
> + */
> + if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> + ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node))
> + return true;
> +
> + /*
> + * 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 effecting real-time
s/effecting/degrading/ to be more precise.
> + * responsiveness. This applies to all CONFIG_RCU_BOOST=y kernels,
> + * not just PREEMPT_RT.
> + */
> + if (IS_ENABLED(CONFIG_RCU_BOOST) && irqs_were_disabled && t->rcu_blocked_node)
> + return true;
> +
> + return false;
> +}
> +
> /*
> * Handle special cases during rcu_read_unlock(), such as needing to
> * notify RCU core processing or task having blocked during the RCU
> @@ -666,18 +732,14 @@ static void rcu_read_unlock_special(struct task_struct *t)
> local_irq_save(flags);
> irqs_were_disabled = irqs_disabled_flags(flags);
> if (preempt_bh_were_disabled || irqs_were_disabled) {
> - bool expboost; // Expedited GP in flight or possible boosting.
> + bool needs_exp; // Expedited handling needed.
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> struct rcu_node *rnp = rdp->mynode;
>
> - expboost = (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks)) ||
> - (rdp->grpmask & READ_ONCE(rnp->expmask)) ||
> - (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> - ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node)) ||
> - (IS_ENABLED(CONFIG_RCU_BOOST) && irqs_were_disabled &&
> - t->rcu_blocked_node);
> + needs_exp = rcu_unlock_needs_exp_handling(t, rdp, rnp, irqs_were_disabled);
> +
> // Need to defer quiescent state until everything is enabled.
> - if (use_softirq && (in_hardirq() || (expboost && !irqs_were_disabled))) {
> + if (use_softirq && (in_hardirq() || (needs_exp && !irqs_were_disabled))) {
> // Using softirq, safe to awaken, and either the
> // wakeup is free or there is either an expedited
> // GP in flight or a potential need to deboost.
> @@ -690,7 +752,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> set_tsk_need_resched(current);
> set_preempt_need_resched();
> if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
> - expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
> + needs_exp && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
> // Get scheduler to re-evaluate and call hooks.
> // If !IRQ_WORK, FQS scan will eventually IPI.
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special()
2025-07-06 17:18 ` Paul E. McKenney
@ 2025-07-06 19:16 ` Joel Fernandes
2025-07-07 4:20 ` Paul E. McKenney
0 siblings, 1 reply; 14+ messages in thread
From: Joel Fernandes @ 2025-07-06 19:16 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Sebastian Andrzej Siewior, Clark Williams, rcu, linux-rt-devel
Hi Paul,
On 7/6/2025 1:18 PM, Paul E. McKenney wrote:
> On Sat, Jul 05, 2025 at 04:39:16PM -0400, Joel Fernandes wrote:
>> Extract the complex expedited handling condition in rcu_read_unlock_special()
>> into a separate function rcu_unlock_needs_exp_handling() with detailed
>> comments explaining each condition.
>>
>> This improves code readability. No functional change intended.
>
> Very nice!!!
Thanks!
>
> Some questions and comments interspersed below.
I replied inline below:
>
> Thanx, Paul
>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> kernel/rcu/tree_plugin.h | 80 +++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 71 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>> index baf57745b42f..8504d95bb35b 100644
>> --- a/kernel/rcu/tree_plugin.h
>> +++ b/kernel/rcu/tree_plugin.h
>> @@ -647,6 +647,72 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>> local_irq_restore(flags);
>> }
>>
>> +/*
>> + * Check if expedited grace period processing during unlock is needed.
>> + *
>> + * This function determines whether expedited handling is required based on:
>> + * 1. Task blocking an expedited grace period
>
> This is a heuristic. What we are actually checking is whether the task
> is blocking *some* grace period and whether at least one task (maybe
> this one, maybe not) is blocking an expedited grace period.
Makes sense, I changed this to:
* 1. Task blocking an expedited grace period (based on a heuristic, could be
* false-positive, see below.)
And the below comment to:
/*
* Check if this task is blocking an expedited grace period. If the
* task was preempted within an RCU read-side critical section and is
* on the expedited grace period blockers list (exp_tasks), we need
* expedited handling to unblock the expedited GP. This is not an exact
* check because 't' might not be on the exp_tasks list at all - its
* just a fast heuristic that can be false-positive sometimes.
*/
if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
return true;
Hope that looks Ok.
>
> Why not an exact check? Because that would mean traversing the list
> starting at ->exp_tasks, and that list could potentially contain every
> task in the system. And I have received bug reports encountered on
> systems with hundreds of thousands of tasks.
Got it.
>
> I could imagine a more complex data structure that semi-efficiently
> tracked exact information, but I could also imagine this not being worth
> the effort.
>
>> + * 2. CPU participating in an expedited grace period
>> + * 3. Strict grace period mode requiring expedited handling
>> + * 4. RCU priority boosting needs when interrupts were disabled
>
> s/boosting/deboosting/
>
Fixed, thanks.
>
>> + */
>> + if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
>> + return true;
>> +
>> + /*
>> + * Check if this CPU is participating in an expedited grace period.
>> + * The expmask bitmap tracks which CPUs need to check in for the
>> + * current expedited GP. If our CPU's bit is set, we need expedited
>> + * handling to help complete the expedited GP.
>> + */
>> + if (rdp->grpmask & READ_ONCE(rnp->expmask))
>> + return true;
>> +
>> + /*
>> + * In CONFIG_RCU_STRICT_GRACE_PERIOD=y kernels, all grace periods
>> + * are treated as short for testing purposes even if that means
>> + * disturbing the system more. Check if either:
>> + * - This CPU has not yet reported a quiescent state, or
>> + * - This task was preempted within an RCU critical section
>> + * In either case, requird expedited handling for strict GP mode.
>
> s/requird/required/ ;-)
I meant "require" :-D. Will fix.
>
>> + */
>> + if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
>> + ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node))
>> + return true;
>> +
>> + /*
>> + * 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 effecting real-time
>
> s/effecting/degrading/ to be more precise.
>
Fixed, thanks.
- Joel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special()
2025-07-06 19:16 ` Joel Fernandes
@ 2025-07-07 4:20 ` Paul E. McKenney
0 siblings, 0 replies; 14+ messages in thread
From: Paul E. McKenney @ 2025-07-07 4:20 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Sebastian Andrzej Siewior, Clark Williams, rcu, linux-rt-devel
On Sun, Jul 06, 2025 at 03:16:54PM -0400, Joel Fernandes wrote:
> Hi Paul,
>
> On 7/6/2025 1:18 PM, Paul E. McKenney wrote:
> > On Sat, Jul 05, 2025 at 04:39:16PM -0400, Joel Fernandes wrote:
> >> Extract the complex expedited handling condition in rcu_read_unlock_special()
> >> into a separate function rcu_unlock_needs_exp_handling() with detailed
> >> comments explaining each condition.
> >>
> >> This improves code readability. No functional change intended.
> >
> > Very nice!!!
>
> Thanks!
>
> >
> > Some questions and comments interspersed below.
>
> I replied inline below:
>
> >
> > Thanx, Paul
> >
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >> ---
> >> kernel/rcu/tree_plugin.h | 80 +++++++++++++++++++++++++++++++++++-----
> >> 1 file changed, 71 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> >> index baf57745b42f..8504d95bb35b 100644
> >> --- a/kernel/rcu/tree_plugin.h
> >> +++ b/kernel/rcu/tree_plugin.h
> >> @@ -647,6 +647,72 @@ static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> >> local_irq_restore(flags);
> >> }
> >>
> >> +/*
> >> + * Check if expedited grace period processing during unlock is needed.
> >> + *
> >> + * This function determines whether expedited handling is required based on:
> >> + * 1. Task blocking an expedited grace period
> >
> > This is a heuristic. What we are actually checking is whether the task
> > is blocking *some* grace period and whether at least one task (maybe
> > this one, maybe not) is blocking an expedited grace period.
>
> Makes sense, I changed this to:
>
> * 1. Task blocking an expedited grace period (based on a heuristic, could be
> * false-positive, see below.)
>
> And the below comment to:
>
> /*
> * Check if this task is blocking an expedited grace period. If the
> * task was preempted within an RCU read-side critical section and is
> * on the expedited grace period blockers list (exp_tasks), we need
> * expedited handling to unblock the expedited GP. This is not an exact
> * check because 't' might not be on the exp_tasks list at all - its
> * just a fast heuristic that can be false-positive sometimes.
> */
> if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
> return true;
>
> Hope that looks Ok.
Looks good, thank you!
Thanx, Paul
> > Why not an exact check? Because that would mean traversing the list
> > starting at ->exp_tasks, and that list could potentially contain every
> > task in the system. And I have received bug reports encountered on
> > systems with hundreds of thousands of tasks.
>
> Got it.
>
> >
> > I could imagine a more complex data structure that semi-efficiently
> > tracked exact information, but I could also imagine this not being worth
> > the effort.
> >
> >> + * 2. CPU participating in an expedited grace period
> >> + * 3. Strict grace period mode requiring expedited handling
> >> + * 4. RCU priority boosting needs when interrupts were disabled
> >
> > s/boosting/deboosting/
> >
>
> Fixed, thanks.
>
> >
> >> + */
> >> + if (t->rcu_blocked_node && READ_ONCE(t->rcu_blocked_node->exp_tasks))
> >> + return true;
> >> +
> >> + /*
> >> + * Check if this CPU is participating in an expedited grace period.
> >> + * The expmask bitmap tracks which CPUs need to check in for the
> >> + * current expedited GP. If our CPU's bit is set, we need expedited
> >> + * handling to help complete the expedited GP.
> >> + */
> >> + if (rdp->grpmask & READ_ONCE(rnp->expmask))
> >> + return true;
> >> +
> >> + /*
> >> + * In CONFIG_RCU_STRICT_GRACE_PERIOD=y kernels, all grace periods
> >> + * are treated as short for testing purposes even if that means
> >> + * disturbing the system more. Check if either:
> >> + * - This CPU has not yet reported a quiescent state, or
> >> + * - This task was preempted within an RCU critical section
> >> + * In either case, requird expedited handling for strict GP mode.
> >
> > s/requird/required/ ;-)
>
> I meant "require" :-D. Will fix.
>
> >
> >> + */
> >> + if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> >> + ((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node))
> >> + return true;
> >> +
> >> + /*
> >> + * 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 effecting real-time
> >
> > s/effecting/degrading/ to be more precise.
> >
>
> Fixed, thanks.
>
> - Joel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 3/3] rcu: Remove redundant check for irq state during unlock
2025-07-05 20:39 [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
2025-07-05 20:39 ` [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special() Joel Fernandes
@ 2025-07-05 20:39 ` Joel Fernandes
2025-07-05 20:41 ` [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Joel Fernandes @ 2025-07-05 20:39 UTC (permalink / raw)
To: linux-kernel, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Boqun Feng,
Uladzislau Rezki, Steven Rostedt, Mathieu Desnoyers,
Lai Jiangshan, Zqiang, Sebastian Andrzej Siewior, Clark Williams
Cc: rcu, linux-rt-devel
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.
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@nvidia.com>
---
kernel/rcu/tree_plugin.h | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 8504d95bb35b..112973ecebb8 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.
@@ -692,7 +690,7 @@ static bool rcu_unlock_needs_exp_handling(struct task_struct *t,
* disturbing the system more. Check if either:
* - This CPU has not yet reported a quiescent state, or
* - This task was preempted within an RCU critical section
- * In either case, requird expedited handling for strict GP mode.
+ * In either case, require expedited handling for strict GP mode.
*/
if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
((rdp->grpmask & READ_ONCE(rnp->qsmask)) || t->rcu_blocked_node))
@@ -700,14 +698,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 effecting real-time
- * responsiveness. This applies to all CONFIG_RCU_BOOST=y kernels,
- * not just PREEMPT_RT.
+ * 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 effecting real-time
+ * responsiveness. This applies to all 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;
@@ -736,7 +734,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.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-05 20:39 [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
2025-07-05 20:39 ` [PATCH RFC 2/3] rcu: Refactor expedited handling check in rcu_read_unlock_special() Joel Fernandes
2025-07-05 20:39 ` [PATCH RFC 3/3] rcu: Remove redundant check for irq state during unlock Joel Fernandes
@ 2025-07-05 20:41 ` Joel Fernandes
2025-07-06 17:08 ` Paul E. McKenney
2025-07-07 13:26 ` qiang.zhang
4 siblings, 0 replies; 14+ messages in thread
From: Joel Fernandes @ 2025-07-05 20:41 UTC (permalink / raw)
To: linux-kernel, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Josh Triplett, Boqun Feng, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang
Cc: rcu
Bleh, my commit message got lost, but hey this is an RFC ;-)
For context, it repairs this issue:
https://lore.kernel.org/all/9acd5f9f-6732-7701-6880-4b51190aa070@huawei.com/
Will add commit message when sending non-RFC later.
thanks,
- Joel
On 7/5/2025 4:39 PM, Joel Fernandes wrote:
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> kernel/rcu/tree.h | 11 ++++++++++-
> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
> 2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 3830c19cf2f6..f8f612269e6e 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -174,6 +174,15 @@ struct rcu_snap_record {
> unsigned long jiffies; /* Track jiffies value */
> };
>
> +/*
> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
> + * It can be in one of the following states:
> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
> + */
> +#define DEFER_QS_IDLE 0
> +#define DEFER_QS_PENDING 1
> +
> /* Per-CPU data for read-copy update. */
> struct rcu_data {
> /* 1) quiescent-state and grace-period handling : */
> @@ -192,7 +201,7 @@ struct rcu_data {
> /* during and after the last grace */
> /* period it is aware of. */
> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
> + int defer_qs_iw_pending; /* Scheduler attention pending? */
> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>
> /* 2) batch handling */
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index dd1c156c1759..baf57745b42f 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> struct rcu_node *rnp;
> union rcu_special special;
>
> + rdp = this_cpu_ptr(&rcu_data);
> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
> +
> /*
> * If RCU core is waiting for this CPU to exit its critical section,
> * report the fact that it has exited. Because irqs are disabled,
> * t->rcu_read_unlock_special cannot change.
> */
> special = t->rcu_read_unlock_special;
> - rdp = this_cpu_ptr(&rcu_data);
> if (!special.s && !rdp->cpu_no_qs.b.exp) {
> local_irq_restore(flags);
> return;
> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
> */
> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> {
> - unsigned long flags;
> - struct rcu_data *rdp;
> + volatile unsigned long flags;
> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
> local_irq_save(flags);
> - rdp->defer_qs_iw_pending = false;
> +
> + /*
> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
> + local_irq_restore(flags);
> + return;
> + }
> local_irq_restore(flags);
> }
>
> @@ -675,7 +690,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> set_tsk_need_resched(current);
> set_preempt_need_resched();
> if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
> - expboost && !rdp->defer_qs_iw_pending && cpu_online(rdp->cpu)) {
> + expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
> // Get scheduler to re-evaluate and call hooks.
> // If !IRQ_WORK, FQS scan will eventually IPI.
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> @@ -685,7 +700,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> else
> init_irq_work(&rdp->defer_qs_iw,
> rcu_preempt_deferred_qs_handler);
> - rdp->defer_qs_iw_pending = true;
> + rdp->defer_qs_iw_pending = DEFER_QS_PENDING;
> irq_work_queue_on(&rdp->defer_qs_iw, rdp->cpu);
> }
> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-05 20:39 [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
` (2 preceding siblings ...)
2025-07-05 20:41 ` [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
@ 2025-07-06 17:08 ` Paul E. McKenney
2025-07-06 17:13 ` Joel Fernandes
2025-07-07 13:26 ` qiang.zhang
4 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2025-07-06 17:08 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, rcu
On Sat, Jul 05, 2025 at 04:39:15PM -0400, Joel Fernandes wrote:
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Definitely headed in the right direction, though it does need just a
little bit more detail in the commit log. ;-)
Also a few comments and questions interspersed below.
Thanx, Paul
> ---
> kernel/rcu/tree.h | 11 ++++++++++-
> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
> 2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 3830c19cf2f6..f8f612269e6e 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -174,6 +174,15 @@ struct rcu_snap_record {
> unsigned long jiffies; /* Track jiffies value */
> };
>
> +/*
> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
> + * It can be in one of the following states:
> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
> + */
> +#define DEFER_QS_IDLE 0
> +#define DEFER_QS_PENDING 1
Having names for the states is good!
> +
> /* Per-CPU data for read-copy update. */
> struct rcu_data {
> /* 1) quiescent-state and grace-period handling : */
> @@ -192,7 +201,7 @@ struct rcu_data {
> /* during and after the last grace */
> /* period it is aware of. */
> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
> + int defer_qs_iw_pending; /* Scheduler attention pending? */
> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>
> /* 2) batch handling */
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index dd1c156c1759..baf57745b42f 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> struct rcu_node *rnp;
> union rcu_special special;
>
> + rdp = this_cpu_ptr(&rcu_data);
> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
Good, this is where the request actually gets serviced.
> +
> /*
> * If RCU core is waiting for this CPU to exit its critical section,
> * report the fact that it has exited. Because irqs are disabled,
> * t->rcu_read_unlock_special cannot change.
> */
> special = t->rcu_read_unlock_special;
> - rdp = this_cpu_ptr(&rcu_data);
> if (!special.s && !rdp->cpu_no_qs.b.exp) {
> local_irq_restore(flags);
> return;
> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
> */
> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> {
> - unsigned long flags;
> - struct rcu_data *rdp;
> + volatile unsigned long flags;
I don't understand why this wants to be volatile.
Unless maybe you want to make sure that gdb can see it, in
which case, is there an existing Kconfig option for that? Maybe
CONFIG_DEBUG_INFO_NONE=n?
> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
> local_irq_save(flags);
> - rdp->defer_qs_iw_pending = false;
> +
> + /*
> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
Shouldn't we have just this WRITE_ONCE() in this then-clause?
> + local_irq_restore(flags);
> + return;
> + }
> local_irq_restore(flags);
> }
>
> @@ -675,7 +690,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> set_tsk_need_resched(current);
> set_preempt_need_resched();
> if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
> - expboost && !rdp->defer_qs_iw_pending && cpu_online(rdp->cpu)) {
> + expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
> // Get scheduler to re-evaluate and call hooks.
> // If !IRQ_WORK, FQS scan will eventually IPI.
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> @@ -685,7 +700,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> else
> init_irq_work(&rdp->defer_qs_iw,
> rcu_preempt_deferred_qs_handler);
> - rdp->defer_qs_iw_pending = true;
> + rdp->defer_qs_iw_pending = DEFER_QS_PENDING;
> irq_work_queue_on(&rdp->defer_qs_iw, rdp->cpu);
> }
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-06 17:08 ` Paul E. McKenney
@ 2025-07-06 17:13 ` Joel Fernandes
2025-07-06 17:26 ` Paul E. McKenney
0 siblings, 1 reply; 14+ messages in thread
From: Joel Fernandes @ 2025-07-06 17:13 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, rcu
On 7/6/2025 1:08 PM, Paul E. McKenney wrote:
> On Sat, Jul 05, 2025 at 04:39:15PM -0400, Joel Fernandes wrote:
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> Definitely headed in the right direction, though it does need just a
> little bit more detail in the commit log. ;-)
>
> Also a few comments and questions interspersed below.
>
> Thanx, Paul
>
>> ---
>> kernel/rcu/tree.h | 11 ++++++++++-
>> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
>> 2 files changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>> index 3830c19cf2f6..f8f612269e6e 100644
>> --- a/kernel/rcu/tree.h
>> +++ b/kernel/rcu/tree.h
>> @@ -174,6 +174,15 @@ struct rcu_snap_record {
>> unsigned long jiffies; /* Track jiffies value */
>> };
>>
>> +/*
>> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
>> + * It can be in one of the following states:
>> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
>> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
>> + */
>> +#define DEFER_QS_IDLE 0
>> +#define DEFER_QS_PENDING 1
>
> Having names for the states is good!
>
>> +
>> /* Per-CPU data for read-copy update. */
>> struct rcu_data {
>> /* 1) quiescent-state and grace-period handling : */
>> @@ -192,7 +201,7 @@ struct rcu_data {
>> /* during and after the last grace */
>> /* period it is aware of. */
>> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
>> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
>> + int defer_qs_iw_pending; /* Scheduler attention pending? */
>> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>>
>> /* 2) batch handling */
>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>> index dd1c156c1759..baf57745b42f 100644
>> --- a/kernel/rcu/tree_plugin.h
>> +++ b/kernel/rcu/tree_plugin.h
>> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>> struct rcu_node *rnp;
>> union rcu_special special;
>>
>> + rdp = this_cpu_ptr(&rcu_data);
>> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
>> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
>
> Good, this is where the request actually gets serviced.
>
>> +
>> /*
>> * If RCU core is waiting for this CPU to exit its critical section,
>> * report the fact that it has exited. Because irqs are disabled,
>> * t->rcu_read_unlock_special cannot change.
>> */
>> special = t->rcu_read_unlock_special;
>> - rdp = this_cpu_ptr(&rcu_data);
>> if (!special.s && !rdp->cpu_no_qs.b.exp) {
>> local_irq_restore(flags);
>> return;
>> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
>> */
>> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>> {
>> - unsigned long flags;
>> - struct rcu_data *rdp;
>> + volatile unsigned long flags;
>
> I don't understand why this wants to be volatile.
>
> Unless maybe you want to make sure that gdb can see it, in
> which case, is there an existing Kconfig option for that? Maybe
> CONFIG_DEBUG_INFO_NONE=n?
This does not need to be volatile, sorry it was an older remnant (back when the
handler was a NOOP in the v1, and I was afraid of compiler optimizations ;-)).
But its no longer needed so I shall drop it (the volatile) :)
>
>> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>>
>> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
>> local_irq_save(flags);
>> - rdp->defer_qs_iw_pending = false;
>> +
>> + /*
>> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
>> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
>
> Shouldn't we have just this WRITE_ONCE() in this then-clause?
No, because if we let the IRQ work handler do that before we can execute
rcu_preempt_deferred_qs_handler(), then it will cause infinite recursion,
because an RCU read-side critical section can again try to queue the IRQ work
(before entering the scheduler). Also testing shows doing that will reproduce
the hang we're fixing.
I think we should rename defer_qs_iw_pending to defer_qs_pending to better
clarify that we are tracking the "Deferred QS" reporting than if the IRQ work
actually ran?
Thanks,
- Joel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-06 17:13 ` Joel Fernandes
@ 2025-07-06 17:26 ` Paul E. McKenney
2025-07-06 18:37 ` Joel Fernandes
0 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2025-07-06 17:26 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, rcu
On Sun, Jul 06, 2025 at 01:13:31PM -0400, Joel Fernandes wrote:
> On 7/6/2025 1:08 PM, Paul E. McKenney wrote:
> > On Sat, Jul 05, 2025 at 04:39:15PM -0400, Joel Fernandes wrote:
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >
> > Definitely headed in the right direction, though it does need just a
> > little bit more detail in the commit log. ;-)
> >
> > Also a few comments and questions interspersed below.
> >
> > Thanx, Paul
> >
> >> ---
> >> kernel/rcu/tree.h | 11 ++++++++++-
> >> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
> >> 2 files changed, 32 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> >> index 3830c19cf2f6..f8f612269e6e 100644
> >> --- a/kernel/rcu/tree.h
> >> +++ b/kernel/rcu/tree.h
> >> @@ -174,6 +174,15 @@ struct rcu_snap_record {
> >> unsigned long jiffies; /* Track jiffies value */
> >> };
> >>
> >> +/*
> >> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
> >> + * It can be in one of the following states:
> >> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
> >> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
> >> + */
> >> +#define DEFER_QS_IDLE 0
> >> +#define DEFER_QS_PENDING 1
> >
> > Having names for the states is good!
> >
> >> +
> >> /* Per-CPU data for read-copy update. */
> >> struct rcu_data {
> >> /* 1) quiescent-state and grace-period handling : */
> >> @@ -192,7 +201,7 @@ struct rcu_data {
> >> /* during and after the last grace */
> >> /* period it is aware of. */
> >> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
> >> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
> >> + int defer_qs_iw_pending; /* Scheduler attention pending? */
> >> struct work_struct strict_work; /* Schedule readers for strict GPs. */
> >>
> >> /* 2) batch handling */
> >> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> >> index dd1c156c1759..baf57745b42f 100644
> >> --- a/kernel/rcu/tree_plugin.h
> >> +++ b/kernel/rcu/tree_plugin.h
> >> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> >> struct rcu_node *rnp;
> >> union rcu_special special;
> >>
> >> + rdp = this_cpu_ptr(&rcu_data);
> >> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
> >> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
> >
> > Good, this is where the request actually gets serviced.
> >
> >> +
> >> /*
> >> * If RCU core is waiting for this CPU to exit its critical section,
> >> * report the fact that it has exited. Because irqs are disabled,
> >> * t->rcu_read_unlock_special cannot change.
> >> */
> >> special = t->rcu_read_unlock_special;
> >> - rdp = this_cpu_ptr(&rcu_data);
> >> if (!special.s && !rdp->cpu_no_qs.b.exp) {
> >> local_irq_restore(flags);
> >> return;
> >> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
> >> */
> >> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> >> {
> >> - unsigned long flags;
> >> - struct rcu_data *rdp;
> >> + volatile unsigned long flags;
> >
> > I don't understand why this wants to be volatile.
> >
> > Unless maybe you want to make sure that gdb can see it, in
> > which case, is there an existing Kconfig option for that? Maybe
> > CONFIG_DEBUG_INFO_NONE=n?
>
> This does not need to be volatile, sorry it was an older remnant (back when the
> handler was a NOOP in the v1, and I was afraid of compiler optimizations ;-)).
> But its no longer needed so I shall drop it (the volatile) :)
Whew! ;-)
> >> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >>
> >> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
> >> local_irq_save(flags);
> >> - rdp->defer_qs_iw_pending = false;
> >> +
> >> + /*
> >> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
> >> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
> >
> > Shouldn't we have just this WRITE_ONCE() in this then-clause?
>
> No, because if we let the IRQ work handler do that before we can execute
> rcu_preempt_deferred_qs_handler(), then it will cause infinite recursion,
> because an RCU read-side critical section can again try to queue the IRQ work
> (before entering the scheduler). Also testing shows doing that will reproduce
> the hang we're fixing.
>
> I think we should rename defer_qs_iw_pending to defer_qs_pending to better
> clarify that we are tracking the "Deferred QS" reporting than if the IRQ work
> actually ran?
Here is the patch:
+ if (rcu_preempt_depth() > 0) {
+ WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
+ local_irq_restore(flags);
+ return;
+ }
local_irq_restore(flags);
}
After the WRITE_ONCE, you restore interrupts and return. Which is also
what would happen if there was only the WRITE_ONCE() in the then-clause,
correct?
Or am I missing something subtle here?
Thanx, Paul
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-06 17:26 ` Paul E. McKenney
@ 2025-07-06 18:37 ` Joel Fernandes
0 siblings, 0 replies; 14+ messages in thread
From: Joel Fernandes @ 2025-07-06 18:37 UTC (permalink / raw)
To: paulmck
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, rcu
> On Jul 6, 2025, at 1:26 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Sun, Jul 06, 2025 at 01:13:31PM -0400, Joel Fernandes wrote:
>>> On 7/6/2025 1:08 PM, Paul E. McKenney wrote:
>>> On Sat, Jul 05, 2025 at 04:39:15PM -0400, Joel Fernandes wrote:
>>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>
>>> Definitely headed in the right direction, though it does need just a
>>> little bit more detail in the commit log. ;-)
>>>
>>> Also a few comments and questions interspersed below.
>>>
>>> Thanx, Paul
>>>
>>>> ---
>>>> kernel/rcu/tree.h | 11 ++++++++++-
>>>> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
>>>> 2 files changed, 32 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>>>> index 3830c19cf2f6..f8f612269e6e 100644
>>>> --- a/kernel/rcu/tree.h
>>>> +++ b/kernel/rcu/tree.h
>>>> @@ -174,6 +174,15 @@ struct rcu_snap_record {
>>>> unsigned long jiffies; /* Track jiffies value */
>>>> };
>>>>
>>>> +/*
>>>> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
>>>> + * It can be in one of the following states:
>>>> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
>>>> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
>>>> + */
>>>> +#define DEFER_QS_IDLE 0
>>>> +#define DEFER_QS_PENDING 1
>>>
>>> Having names for the states is good!
>>>
>>>> +
>>>> /* Per-CPU data for read-copy update. */
>>>> struct rcu_data {
>>>> /* 1) quiescent-state and grace-period handling : */
>>>> @@ -192,7 +201,7 @@ struct rcu_data {
>>>> /* during and after the last grace */
>>>> /* period it is aware of. */
>>>> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
>>>> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
>>>> + int defer_qs_iw_pending; /* Scheduler attention pending? */
>>>> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>>>>
>>>> /* 2) batch handling */
>>>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>>>> index dd1c156c1759..baf57745b42f 100644
>>>> --- a/kernel/rcu/tree_plugin.h
>>>> +++ b/kernel/rcu/tree_plugin.h
>>>> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>>>> struct rcu_node *rnp;
>>>> union rcu_special special;
>>>>
>>>> + rdp = this_cpu_ptr(&rcu_data);
>>>> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
>>>> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
>>>
>>> Good, this is where the request actually gets serviced.
>>>
>>>> +
>>>> /*
>>>> * If RCU core is waiting for this CPU to exit its critical section,
>>>> * report the fact that it has exited. Because irqs are disabled,
>>>> * t->rcu_read_unlock_special cannot change.
>>>> */
>>>> special = t->rcu_read_unlock_special;
>>>> - rdp = this_cpu_ptr(&rcu_data);
>>>> if (!special.s && !rdp->cpu_no_qs.b.exp) {
>>>> local_irq_restore(flags);
>>>> return;
>>>> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
>>>> */
>>>> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>>>> {
>>>> - unsigned long flags;
>>>> - struct rcu_data *rdp;
>>>> + volatile unsigned long flags;
>>>
>>> I don't understand why this wants to be volatile.
>>>
>>> Unless maybe you want to make sure that gdb can see it, in
>>> which case, is there an existing Kconfig option for that? Maybe
>>> CONFIG_DEBUG_INFO_NONE=n?
>>
>> This does not need to be volatile, sorry it was an older remnant (back when the
>> handler was a NOOP in the v1, and I was afraid of compiler optimizations ;-)).
>> But its no longer needed so I shall drop it (the volatile) :)
>
> Whew! ;-)
>
>>>> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>>>>
>>>> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
>>>> local_irq_save(flags);
>>>> - rdp->defer_qs_iw_pending = false;
>>>> +
>>>> + /*
>>>> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
>>>> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
>>>
>>> Shouldn't we have just this WRITE_ONCE() in this then-clause?
>>
>> No, because if we let the IRQ work handler do that before we can execute
>> rcu_preempt_deferred_qs_handler(), then it will cause infinite recursion,
>> because an RCU read-side critical section can again try to queue the IRQ work
>> (before entering the scheduler). Also testing shows doing that will reproduce
>> the hang we're fixing.
>>
>> I think we should rename defer_qs_iw_pending to defer_qs_pending to better
>> clarify that we are tracking the "Deferred QS" reporting than if the IRQ work
>> actually ran?
>
> Here is the patch:
>
> + if (rcu_preempt_depth() > 0) {
> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
> + local_irq_restore(flags);
> + return;
> + }
> local_irq_restore(flags);
> }
>
> After the WRITE_ONCE, you restore interrupts and return. Which is also
> what would happen if there was only the WRITE_ONCE() in the then-clause,
> correct?
>
> Or am I missing something subtle here?
Oh, true! I will delete those 2 lines inside the if block. Thanks!
- Joel
>
> Thanx, Paul
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-05 20:39 [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work Joel Fernandes
` (3 preceding siblings ...)
2025-07-06 17:08 ` Paul E. McKenney
@ 2025-07-07 13:26 ` qiang.zhang
2025-07-07 14:04 ` Paul E. McKenney
4 siblings, 1 reply; 14+ messages in thread
From: qiang.zhang @ 2025-07-07 13:26 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Josh Triplett, Boqun Feng,
Uladzislau Rezki, Steven Rostedt, Mathieu Desnoyers,
Lai Jiangshan
Cc: rcu
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> ---
>
> kernel/rcu/tree.h | 11 ++++++++++-
>
> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
>
> 2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>
> index 3830c19cf2f6..f8f612269e6e 100644
>
> --- a/kernel/rcu/tree.h
>
> +++ b/kernel/rcu/tree.h
>
> @@ -174,6 +174,15 @@ struct rcu_snap_record {
>
> unsigned long jiffies; /* Track jiffies value */
>
> };
>
>
>
> +/*
>
> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
>
> + * It can be in one of the following states:
>
> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
>
> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
>
> + */
>
> +#define DEFER_QS_IDLE 0
>
> +#define DEFER_QS_PENDING 1
>
> +
>
> /* Per-CPU data for read-copy update. */
>
> struct rcu_data {
>
> /* 1) quiescent-state and grace-period handling : */
>
> @@ -192,7 +201,7 @@ struct rcu_data {
>
> /* during and after the last grace */
>
> /* period it is aware of. */
>
> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
>
> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
>
> + int defer_qs_iw_pending; /* Scheduler attention pending? */
>
> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>
>
>
> /* 2) batch handling */
>
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>
> index dd1c156c1759..baf57745b42f 100644
>
> --- a/kernel/rcu/tree_plugin.h
>
> +++ b/kernel/rcu/tree_plugin.h
>
> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>
> struct rcu_node *rnp;
>
> union rcu_special special;
>
>
>
> + rdp = this_cpu_ptr(&rcu_data);
>
> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
>
> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
>
> +
>
> /*
>
> * If RCU core is waiting for this CPU to exit its critical section,
>
> * report the fact that it has exited. Because irqs are disabled,
>
> * t->rcu_read_unlock_special cannot change.
>
> */
>
> special = t->rcu_read_unlock_special;
>
> - rdp = this_cpu_ptr(&rcu_data);
>
> if (!special.s && !rdp->cpu_no_qs.b.exp) {
>
> local_irq_restore(flags);
>
> return;
>
> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
>
> */
>
> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>
> {
>
> - unsigned long flags;
>
> - struct rcu_data *rdp;
>
> + volatile unsigned long flags;
>
> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
>
>
> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
>
> local_irq_save(flags);
>
> - rdp->defer_qs_iw_pending = false;
>
> +
>
> + /*
>
> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
For Preempt-RT kernels, the rcu_preempt_deferred_qs_handler() be invoked
in per-cpu irq_work kthreads, the return value of rcu_preempt_depth()
may always be 0, should we use IRQ_WORK_INIT_HARD() to initialize defer_qs_iw?
Thanks
Zqiang
>
> + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
>
> + local_irq_restore(flags);
>
> + return;
>
> + }
>
> local_irq_restore(flags);
>
> }
>
>
>
> @@ -675,7 +690,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
>
> set_tsk_need_resched(current);
>
> set_preempt_need_resched();
>
> if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
>
> - expboost && !rdp->defer_qs_iw_pending && cpu_online(rdp->cpu)) {
>
> + expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
>
> // Get scheduler to re-evaluate and call hooks.
>
> // If !IRQ_WORK, FQS scan will eventually IPI.
>
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
>
> @@ -685,7 +700,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
>
> else
>
> init_irq_work(&rdp->defer_qs_iw,
>
> rcu_preempt_deferred_qs_handler);
>
> - rdp->defer_qs_iw_pending = true;
>
> + rdp->defer_qs_iw_pending = DEFER_QS_PENDING;
>
> irq_work_queue_on(&rdp->defer_qs_iw, rdp->cpu);
>
> }
>
> }
>
> --
>
> 2.43.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-07 13:26 ` qiang.zhang
@ 2025-07-07 14:04 ` Paul E. McKenney
2025-07-07 14:51 ` Joel Fernandes
0 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2025-07-07 14:04 UTC (permalink / raw)
To: qiang.zhang
Cc: Joel Fernandes, linux-kernel, Frederic Weisbecker,
Neeraj Upadhyay, Josh Triplett, Boqun Feng, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, rcu
On Mon, Jul 07, 2025 at 01:26:56PM +0000, qiang.zhang@linux.dev wrote:
> >
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >
> > ---
> >
> > kernel/rcu/tree.h | 11 ++++++++++-
> >
> > kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
> >
> > 2 files changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> >
> > index 3830c19cf2f6..f8f612269e6e 100644
> >
> > --- a/kernel/rcu/tree.h
> >
> > +++ b/kernel/rcu/tree.h
> >
> > @@ -174,6 +174,15 @@ struct rcu_snap_record {
> >
> > unsigned long jiffies; /* Track jiffies value */
> >
> > };
> >
> >
> >
> > +/*
> >
> > + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
> >
> > + * It can be in one of the following states:
> >
> > + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
> >
> > + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
> >
> > + */
> >
> > +#define DEFER_QS_IDLE 0
> >
> > +#define DEFER_QS_PENDING 1
> >
> > +
> >
> > /* Per-CPU data for read-copy update. */
> >
> > struct rcu_data {
> >
> > /* 1) quiescent-state and grace-period handling : */
> >
> > @@ -192,7 +201,7 @@ struct rcu_data {
> >
> > /* during and after the last grace */
> >
> > /* period it is aware of. */
> >
> > struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
> >
> > - bool defer_qs_iw_pending; /* Scheduler attention pending? */
> >
> > + int defer_qs_iw_pending; /* Scheduler attention pending? */
> >
> > struct work_struct strict_work; /* Schedule readers for strict GPs. */
> >
> >
> >
> > /* 2) batch handling */
> >
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> >
> > index dd1c156c1759..baf57745b42f 100644
> >
> > --- a/kernel/rcu/tree_plugin.h
> >
> > +++ b/kernel/rcu/tree_plugin.h
> >
> > @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> >
> > struct rcu_node *rnp;
> >
> > union rcu_special special;
> >
> >
> >
> > + rdp = this_cpu_ptr(&rcu_data);
> >
> > + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
> >
> > + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
> >
> > +
> >
> > /*
> >
> > * If RCU core is waiting for this CPU to exit its critical section,
> >
> > * report the fact that it has exited. Because irqs are disabled,
> >
> > * t->rcu_read_unlock_special cannot change.
> >
> > */
> >
> > special = t->rcu_read_unlock_special;
> >
> > - rdp = this_cpu_ptr(&rcu_data);
> >
> > if (!special.s && !rdp->cpu_no_qs.b.exp) {
> >
> > local_irq_restore(flags);
> >
> > return;
> >
> > @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
> >
> > */
> >
> > static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
> >
> > {
> >
> > - unsigned long flags;
> >
> > - struct rcu_data *rdp;
> >
> > + volatile unsigned long flags;
> >
> > + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> >
> >
> > - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
> >
> > local_irq_save(flags);
> >
> > - rdp->defer_qs_iw_pending = false;
> >
> > +
> >
> > + /*
> >
> > + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
>
>
> For Preempt-RT kernels, the rcu_preempt_deferred_qs_handler() be invoked
> in per-cpu irq_work kthreads, the return value of rcu_preempt_depth()
> may always be 0, should we use IRQ_WORK_INIT_HARD() to initialize defer_qs_iw?
It sure does look like we need "||" rather than "&&" here in
rcu_read_unlock_special():
if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
IS_ENABLED(CONFIG_PREEMPT_RT))
rdp->defer_qs_iw = IRQ_WORK_INIT_HARD(
rcu_preempt_deferred_qs_handler);
else
init_irq_work(&rdp->defer_qs_iw,
rcu_preempt_deferred_qs_handler);
But it is early in the morning, so I might be missing something.
Thanx, Paul
> Thanks
> Zqiang
>
>
>
> >
> > + WRITE_ONCE(rdp->defer_qs_iw_pending, DEFER_QS_IDLE);
> >
> > + local_irq_restore(flags);
> >
> > + return;
> >
> > + }
> >
> > local_irq_restore(flags);
> >
> > }
> >
> >
> >
> > @@ -675,7 +690,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >
> > set_tsk_need_resched(current);
> >
> > set_preempt_need_resched();
> >
> > if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
> >
> > - expboost && !rdp->defer_qs_iw_pending && cpu_online(rdp->cpu)) {
> >
> > + expboost && rdp->defer_qs_iw_pending != DEFER_QS_PENDING && cpu_online(rdp->cpu)) {
> >
> > // Get scheduler to re-evaluate and call hooks.
> >
> > // If !IRQ_WORK, FQS scan will eventually IPI.
> >
> > if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> >
> > @@ -685,7 +700,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >
> > else
> >
> > init_irq_work(&rdp->defer_qs_iw,
> >
> > rcu_preempt_deferred_qs_handler);
> >
> > - rdp->defer_qs_iw_pending = true;
> >
> > + rdp->defer_qs_iw_pending = DEFER_QS_PENDING;
> >
> > irq_work_queue_on(&rdp->defer_qs_iw, rdp->cpu);
> >
> > }
> >
> > }
> >
> > --
> >
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH RFC 1/3] rcu: Fix rcu_read_unlock() deadloop due to IRQ work
2025-07-07 14:04 ` Paul E. McKenney
@ 2025-07-07 14:51 ` Joel Fernandes
0 siblings, 0 replies; 14+ messages in thread
From: Joel Fernandes @ 2025-07-07 14:51 UTC (permalink / raw)
To: paulmck, qiang.zhang
Cc: linux-kernel, Frederic Weisbecker, Neeraj Upadhyay,
Josh Triplett, Boqun Feng, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, rcu
On 7/7/2025 10:04 AM, Paul E. McKenney wrote:
> On Mon, Jul 07, 2025 at 01:26:56PM +0000, qiang.zhang@linux.dev wrote:
>>>
>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>
>>> ---
>>>
>>> kernel/rcu/tree.h | 11 ++++++++++-
>>>
>>> kernel/rcu/tree_plugin.h | 29 ++++++++++++++++++++++-------
>>>
>>> 2 files changed, 32 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>>>
>>> index 3830c19cf2f6..f8f612269e6e 100644
>>>
>>> --- a/kernel/rcu/tree.h
>>>
>>> +++ b/kernel/rcu/tree.h
>>>
>>> @@ -174,6 +174,15 @@ struct rcu_snap_record {
>>>
>>> unsigned long jiffies; /* Track jiffies value */
>>>
>>> };
>>>
>>>
>>>
>>> +/*
>>>
>>> + * The IRQ work (deferred_qs_iw) is used by RCU to get scheduler's attention.
>>>
>>> + * It can be in one of the following states:
>>>
>>> + * - DEFER_QS_IDLE: An IRQ work was never scheduled.
>>>
>>> + * - DEFER_QS_PENDING: An IRQ work was scheduler but never run.
>>>
>>> + */
>>>
>>> +#define DEFER_QS_IDLE 0
>>>
>>> +#define DEFER_QS_PENDING 1
>>>
>>> +
>>>
>>> /* Per-CPU data for read-copy update. */
>>>
>>> struct rcu_data {
>>>
>>> /* 1) quiescent-state and grace-period handling : */
>>>
>>> @@ -192,7 +201,7 @@ struct rcu_data {
>>>
>>> /* during and after the last grace */
>>>
>>> /* period it is aware of. */
>>>
>>> struct irq_work defer_qs_iw; /* Obtain later scheduler attention. */
>>>
>>> - bool defer_qs_iw_pending; /* Scheduler attention pending? */
>>>
>>> + int defer_qs_iw_pending; /* Scheduler attention pending? */
>>>
>>> struct work_struct strict_work; /* Schedule readers for strict GPs. */
>>>
>>>
>>>
>>> /* 2) batch handling */
>>>
>>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>>>
>>> index dd1c156c1759..baf57745b42f 100644
>>>
>>> --- a/kernel/rcu/tree_plugin.h
>>>
>>> +++ b/kernel/rcu/tree_plugin.h
>>>
>>> @@ -486,13 +486,16 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>>>
>>> struct rcu_node *rnp;
>>>
>>> union rcu_special special;
>>>
>>>
>>>
>>> + rdp = this_cpu_ptr(&rcu_data);
>>>
>>> + if (rdp->defer_qs_iw_pending == DEFER_QS_PENDING)
>>>
>>> + rdp->defer_qs_iw_pending = DEFER_QS_IDLE;
>>>
>>> +
>>>
>>> /*
>>>
>>> * If RCU core is waiting for this CPU to exit its critical section,
>>>
>>> * report the fact that it has exited. Because irqs are disabled,
>>>
>>> * t->rcu_read_unlock_special cannot change.
>>>
>>> */
>>>
>>> special = t->rcu_read_unlock_special;
>>>
>>> - rdp = this_cpu_ptr(&rcu_data);
>>>
>>> if (!special.s && !rdp->cpu_no_qs.b.exp) {
>>>
>>> local_irq_restore(flags);
>>>
>>> return;
>>>
>>> @@ -623,12 +626,24 @@ notrace void rcu_preempt_deferred_qs(struct task_struct *t)
>>>
>>> */
>>>
>>> static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
>>>
>>> {
>>>
>>> - unsigned long flags;
>>>
>>> - struct rcu_data *rdp;
>>>
>>> + volatile unsigned long flags;
>>>
>>> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>>>
>>>
>>>
>>> - rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
>>>
>>> local_irq_save(flags);
>>>
>>> - rdp->defer_qs_iw_pending = false;
>>>
>>> +
>>>
>>> + /*
>>>
>>> + * Requeue the IRQ work on next unlock in 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 (rcu_preempt_depth() > 0) {
>>
>>
>> For Preempt-RT kernels, the rcu_preempt_deferred_qs_handler() be invoked
>> in per-cpu irq_work kthreads, the return value of rcu_preempt_depth()
>> may always be 0, should we use IRQ_WORK_INIT_HARD() to initialize defer_qs_iw?
>
> It sure does look like we need "||" rather than "&&" here in
> rcu_read_unlock_special():
>
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) &&
> IS_ENABLED(CONFIG_PREEMPT_RT))
> rdp->defer_qs_iw = IRQ_WORK_INIT_HARD(
> rcu_preempt_deferred_qs_handler);
> else
> init_irq_work(&rdp->defer_qs_iw,
> rcu_preempt_deferred_qs_handler);
>
Yes, or could we always IRQ_WORK_INIT_HARD()?
For more context, git blame says Z had looked at this before and added
IRQ_WORK_INIT_HARD:
commit f596e2ce1c0f250bb3ecc179f611be37e862635f
Author: Zqiang <qiang1.zhang@intel.com>
Date: Mon Apr 4 07:59:32 2022 +0800
rcu: Use IRQ_WORK_INIT_HARD() to avoid rcu_read_unlock() hangs
When booting kernels built with both CONFIG_RCU_STRICT_GRACE_PERIOD=y
and CONFIG_PREEMPT_RT=y, the rcu_read_unlock_special() function's
invocation of irq_work_queue_on() the init_irq_work() causes the
rcu_preempt_deferred_qs_handler() function to work execute in SCHED_FIFO
irq_work kthreads. Because rcu_read_unlock_special() is invoked on each
rcu_read_unlock() in such kernels, the amount of work just keeps piling
up, resulting in a boot-time hang.
This commit therefore avoids this hang by using IRQ_WORK_INIT_HARD()
instead of init_irq_work(), but only in kernels built with both
CONFIG_PREEMPT_RT=y and CONFIG_RCU_STRICT_GRACE_PERIOD=y.
Signed-off-by: Zqiang <qiang1.zhang@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Btw, once we conclude discussion of how to handle it, if Z could send a patch
based on my rcu/next branch [1], I could apply it to post for further
testing/review.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=rcu/next
thanks,
- Joel
^ permalink raw reply [flat|nested] 14+ messages in thread