lkml.org 
[lkml]   [2022]   [Jan]   [26]   [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/26/2022 1:51 AM, Paul E. McKenney wrote:
> On Mon, Jan 24, 2022 at 10:28:28PM +0530, Mukesh Ojha wrote:
>> On 1/24/2022 10:14 PM, Paul E. McKenney wrote:
>>> On Mon, Jan 24, 2022 at 07:32:01PM +0530, Mukesh Ojha wrote:
>>>> 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 ?
>>> It is currently in -rcu and should also be in -next shortly. Left to
>>> myself, and assuming further testing and reviews all go well, I would
>>> submit it during the upcoming v5.18 merge window.
>>>
>>> Does that work for you? Or do you need it in mainline sooner?
>> Before reporting this issue, we saw only one instance of it.
>> Also got this fix tested with same set of test cases, did not observe any
>> issue as of yet.
>>
>> I would be happy to get a mail once it clear all the testing and get merges
>> to -next. I would cherry-pick it in android branch-5.10.
> It is in -next as of next-20220125.

Thanks :-)


> Thanx, Paul
>
>> -Mukesh
>>
>>> Thanx, Paul
>>>
>>>> -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-26 08:33    [W:0.087 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site