> On Nov 29, 2022, at 3:13 PM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>
> On Tue, Nov 29, 2022 at 03:01:12PM -0500, Joel Fernandes wrote:
>>>> On Nov 29, 2022, at 2:18 PM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>>> On Tue, Nov 29, 2022 at 11:00:05AM -0500, Joel Fernandes wrote:
>>>>>> On Nov 29, 2022, at 10:18 AM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>>>>> On Tue, Nov 29, 2022 at 06:25:04AM +0000, Zhang, Qiang1 wrote:
>>>>>>>> On Nov 28, 2022, at 11:54 PM, Zhang, Qiang1 <qiang1.zhang@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> On Mon, Nov 28, 2022 at 10:34:28PM +0800, Zqiang wrote:
>>>>>>>> Currently, invoke rcu_tasks_rude_wait_gp() to wait one rude
>>>>>>>> RCU-tasks grace period, if __num_online_cpus == 1, will return
>>>>>>>> directly, indicates the end of the rude RCU-task grace period.
>>>>>>>> suppose the system has two cpus, consider the following scenario:
>>>>>>>>
>>>>>>>> CPU0 CPU1 (going offline)
>>>>>>>> migration/1 task:
>>>>>>>> cpu_stopper_thread
>>>>>>>> -> take_cpu_down
>>>>>>>> -> _cpu_disable
>>>>>>>> (dec __num_online_cpus)
>>>>>>>> ->cpuhp_invoke_callback
>>>>>>>> preempt_disable
>>>>>>>> access old_data0
>>>>>>>> task1
>>>>>>>> del old_data0 .....
>>>>>>>> synchronize_rcu_tasks_rude()
>>>>>>>> task1 schedule out
>>>>>>>> ....
>>>>>>>> task2 schedule in
>>>>>>>> rcu_tasks_rude_wait_gp()
>>>>>>>> ->__num_online_cpus == 1
>>>>>>>> ->return
>>>>>>>> ....
>>>>>>>> task1 schedule in
>>>>>>>> ->free old_data0
>>>>>>>> preempt_enable
>>>>>>>>
>>>>>>>> when CPU1 dec __num_online_cpus and __num_online_cpus is equal one,
>>>>>>>> the CPU1 has not finished offline, stop_machine task(migration/1)
>>>>>>>> still running on CPU1, maybe still accessing 'old_data0', but the
>>>>>>>> 'old_data0' has freed on CPU0.
>>>>>>>>
>>>>>>>> This commit add cpus_read_lock/unlock() protection before accessing
>>>>>>>> __num_online_cpus variables, to ensure that the CPU in the offline
>>>>>>>> process has been completed offline.
>>>>>>>>
>>>>>>>> Signed-off-by: Zqiang <qiang1.zhang@xxxxxxxxx>
>>>>>>>>
>>>>>>>> First, good eyes and good catch!!!
>>>>>>>>
>>>>>>>> The purpose of that check for num_online_cpus() is not performance
>>>>>>>> on single-CPU systems, but rather correct operation during early boot.
>>>>>>>> So a simpler way to make that work is to check for RCU_SCHEDULER_RUNNING,
>>>>>>>> for example, as follows:
>>>>>>>>
>>>>>>>> if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING &&
>>>>>>>> num_online_cpus() <= 1)
>>>>>>>> return; // Early boot fastpath for only one CPU.
>>>>>>>
>>>>>>> Hi Paul
>>>>>>>
>>>>>>> During system startup, because the RCU_SCHEDULER_RUNNING is set after starting other CPUs,
>>>>>>>
>>>>>>> CPU0 CPU1
>>>>>>>
>>>>>>> if (rcu_scheduler_active !=
>>>>>>> RCU_SCHEDULER_RUNNING &&
>>>>>>> __num_online_cpus == 1)
>>>>>>> return; inc __num_online_cpus
>>>>>>> (__num_online_cpus == 2)
>>>>>>>
>>>>>>> CPU0 didn't notice the update of the __num_online_cpus variable by CPU1 in time
>>>>>>> Can we move rcu_set_runtime_mode() before smp_init()
>>>>>>> any thoughts?
>>>>>>>
>>>>>>> Is anyone expected to do rcu-tasks operation before the scheduler is running?
>>>>>>
>>>>>> Not sure if such a scenario exists.
>>>>>>
>>>>>>> Typically this requires the tasks to context switch which is a scheduler operation.
>>>>>>>
>>>>>>> If the scheduler is not yet running, then I don’t think missing an update the __num_online_cpus matters since no one does a tasks-RCU synchronize.
>>>>>>
>>>>>> Hi Joel
>>>>>>
>>>>>> After the kernel_init task runs, before calling smp_init() to starting other CPUs,
>>>>>> the scheduler haven been initialization, task context switching can occur.
>>>>>
>>>>> Good catch, thank you both. For some reason, I was thinking that the
>>>>> additional CPUs did not come online until later.
>>>>>
>>>>> So how about this?
>>>>>
>>>>> if (rcu_scheduler_active == RCU_SCHEDULER_INACTIVE)
>>>>> return; // Early boot fastpath.
>>>>>
>>>>> If this condition is true, there is only one CPU and no scheduler,
>>>>> thus no preemption.
>>>>
>>>> Agreed. I was going to suggest exactly this :)
>>>>
>>>> Ack.
>>>> (Replying by phone but feel free to add my reviewed by tag).
>>>
>>> I should add that the downside of this approach is that there is a short
>>> time between the scheduler initializing and workqueues fully initializing
>>> where a critical-path call to synchronize_rcu_tasks() will hang the
>>> system. I do -not- consider this to be a real problem because RCU had
>>> some hundreds of calls to synchronize_rcu() before this became an issue.
>>>
>>> So this should be fine, but please recall this for when/if someone does
>>> stick a synchronize_rcu_tasks() into that short time. ;-)
>>
>> Thanks Paul, but why would anyone want to do sync rcu tasks, before
>> the scheduler is fully initialized?
>
> I could ask that same question of a number of other RCU API members. ;-)
>
>> Maybe we can add a warning here in the if-early-return path, to make
>> sure no such usage slips. And then we can look into someone using it
>> that way, if they ever start using it.
>
> I expect that it would be more work to code and maintain any such warning
> than it would to diagnose the hang, so let's leave it as is.
Makes sense. Works for me and you over to you Qiang! ;-)
Thanks,
- Joel
>
> Thanx, Paul
>
>> Thanks,
>>
>> - Joel
>>
>>>
>>> Thanx, Paul
>>>
>>>> - Joel
>>>>
>>>>
>>>>>
>>>>> Thanx, Paul
>>>>>
>>>>>> Thanks
>>>>>> Zqiang
>>>>>>
>>>>>>>
>>>>>>> Or did I miss something?
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> Zqiang
>>>>>>>
>>>>>>>>
>>>>>>>> This works because rcu_scheduler_active is set to RCU_SCHEDULER_RUNNING
>>>>>>>> long before it is possible to offline CPUs.
>>>>>>>>
>>>>>>>> Yes, schedule_on_each_cpu() does do cpus_read_lock(), again, good eyes,
>>>>>>>> and it also unnecessarily does the schedule_work_on() the current CPU,
>>>>>>>> but the code calling synchronize_rcu_tasks_rude() is on high-overhead
>>>>>>>> code paths, so this overhead is down in the noise.
>>>>>>>>
>>>>>>>> Until further notice, anyway.
>>>>>>>>
>>>>>>>> So simplicity is much more important than performance in this code.
>>>>>>>> So just adding the check for RCU_SCHEDULER_RUNNING should fix this,
>>>>>>>> unless I am missing something (always possible!).
>>>>>>>>
>>>>>>>> Thanx, Paul
>>>>>>>>
>>>>>>>> ---
>>>>>>>> kernel/rcu/tasks.h | 20 ++++++++++++++++++--
>>>>>>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
>>>>>>>> index 4a991311be9b..08e72c6462d8 100644
>>>>>>>> --- a/kernel/rcu/tasks.h
>>>>>>>> +++ b/kernel/rcu/tasks.h
>>>>>>>> @@ -1033,14 +1033,30 @@ static void rcu_tasks_be_rude(struct work_struct *work)
>>>>>>>> {
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static DEFINE_PER_CPU(struct work_struct, rude_work);
>>>>>>>> +
>>>>>>>> // Wait for one rude RCU-tasks grace period.
>>>>>>>> static void rcu_tasks_rude_wait_gp(struct rcu_tasks *rtp)
>>>>>>>> {
>>>>>>>> + int cpu;
>>>>>>>> + struct work_struct *work;
>>>>>>>> +
>>>>>>>> + cpus_read_lock();
>>>>>>>> if (num_online_cpus() <= 1)
>>>>>>>> - return; // Fastpath for only one CPU.
>>>>>>>> + goto end;// Fastpath for only one CPU.
>>>>>>>>
>>>>>>>> rtp->n_ipis += cpumask_weight(cpu_online_mask);
>>>>>>>> - schedule_on_each_cpu(rcu_tasks_be_rude);
>>>>>>>> + for_each_online_cpu(cpu) {
>>>>>>>> + work = per_cpu_ptr(&rude_work, cpu);
>>>>>>>> + INIT_WORK(work, rcu_tasks_be_rude);
>>>>>>>> + schedule_work_on(cpu, work);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + for_each_online_cpu(cpu)
>>>>>>>> + flush_work(per_cpu_ptr(&rude_work, cpu));
>>>>>>>> +
>>>>>>>> +end:
>>>>>>>> + cpus_read_unlock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> void call_rcu_tasks_rude(struct rcu_head *rhp, rcu_callback_t func);
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
