lkml.org 
[lkml]   [2022]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: synchronize_rcu_expedited gets stuck in hotplug path
From

On 1/19/2022 3:11 AM, Paul E. McKenney wrote:
> On Tue, Jan 18, 2022 at 10:11:34AM -1000, Tejun Heo wrote:
>> Hello,
>>
>> On Tue, Jan 18, 2022 at 12:06:46PM -0800, Paul E. McKenney wrote:
>>> Interesting. Adding Tejun and Lai on CC for their perspective.
>>>
>>> As you say, the incoming CPU invoked synchronize_rcu_expedited() which
>>> in turn invoked queue_work(). By default, workqueues will of course
>>> queue that work on the current CPU. But in this case, the CPU's bit
>>> is not yet set in the cpu_active_mask. Thus, a workqueue scheduled on
>>> the incoming CPU won't be invoked until CPUHP_AP_ACTIVE, which won't
>>> be reached until after the grace period ends, which cannot happen until
>>> the workqueue handler is invoked.
>>>
>>> I could imagine doing something as shown in the (untested) patch below,
>>> but first does this help?
>>>
>>> If it does help, would this sort of check be appropriate here or
>>> should it instead go into workqueues?
>> Maybe it can be solved by rearranging the hotplug sequence but it's fragile
>> to schedule per-cpu work items from hotplug paths. Maybe the whole issue can
>> be side-stepped by making synchronize_rcu_expedited() use unbound workqueue
>> instead? Does it require to be per-cpu?
> Good point!
>
> And now that you mention it, RCU expedited grace periods already avoid
> using workqueues during early boot. The (again untested) patch below
> extends that approach to incoming CPUs.
>
> Thoughts?

Hi Paul,

We are not seeing the issue after this patch.
Can we merge this patch ?

-Mukesh

>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index 60197ea24ceb9..1a45667402260 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -816,7 +816,7 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
> */
> void synchronize_rcu_expedited(void)
> {
> - bool boottime = (rcu_scheduler_active == RCU_SCHEDULER_INIT);
> + bool no_wq;
> struct rcu_exp_work rew;
> struct rcu_node *rnp;
> unsigned long s;
> @@ -841,9 +841,15 @@ void synchronize_rcu_expedited(void)
> if (exp_funnel_lock(s))
> return; /* Someone else did our work for us. */
>
> + /* Don't use workqueue during boot or from an incoming CPU. */
> + preempt_disable();
> + no_wq = rcu_scheduler_active == RCU_SCHEDULER_INIT ||
> + !cpumask_test_cpu(smp_processor_id(), cpu_active_mask);
> + preempt_enable();
> +
> /* Ensure that load happens before action based on it. */
> - if (unlikely(boottime)) {
> - /* Direct call during scheduler init and early_initcalls(). */
> + if (unlikely(no_wq)) {
> + /* Direct call for scheduler init, early_initcall()s, and incoming CPUs. */
> rcu_exp_sel_wait_wake(s);
> } else {
> /* Marshall arguments & schedule the expedited grace period. */
> @@ -861,7 +867,7 @@ void synchronize_rcu_expedited(void)
> /* Let the next expedited grace period start. */
> mutex_unlock(&rcu_state.exp_mutex);
>
> - if (likely(!boottime))
> + if (likely(!no_wq))
> destroy_work_on_stack(&rew.rew_work);
> }
> EXPORT_SYMBOL_GPL(synchronize_rcu_expedited);

\
 
 \ /
  Last update: 2022-01-24 15:03    [W:0.257 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site