Message-Id: <24EC376D-B542-4E3C-BC10-3E81F2F2F49C@joelfernandes.org>
Date:   Tue, 29 Nov 2022 00:59:45 -0500
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Zhang, Qiang1" <qiang1.zhang@...el.com>
Cc:     paulmck@...nel.org, frederic@...nel.org, quic_neeraju@...cinc.com,
        rcu@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rcu-tasks: Make rude RCU-Tasks work well with CPU hotplug



> On Nov 28, 2022, at 11:54 PM, Zhang, Qiang1 <qiang1.zhang@...el.com> 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@...el.com>
>> 
>> 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? 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.

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

Powered by blists - more mailing lists