Re: synchronize_rcu_expedited gets stuck in hotplug path

[Mukesh Ojha <quic_mojha@quicinc.com>]

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);