Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v2] rcu-tasks: Make rude RCU-Tasks work well with CPU hotplug
Date:   Tue, 29 Nov 2022 00:59:45 -0500
Message-Id: <24EC376D-B542-4E3C-BC10-3E81F2F2F49C@joelfernandes.org>
References: <PH0PR11MB588001E6982A9DC32F93FD7ADA129@PH0PR11MB5880.namprd11.prod.outlook.com>
Cc:     paulmck@kernel.org, frederic@kernel.org, quic_neeraju@quicinc.com,
        rcu@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <PH0PR11MB588001E6982A9DC32F93FD7ADA129@PH0PR11MB5880.namprd11.prod.outlook.com>
To:     "Zhang, Qiang1" <qiang1.zhang@intel.com>
Precedence: bulk


> On Nov 28, 2022, at 11:54 PM, Zhang, Qiang1 <qiang1.zhang@intel.com> wrote=
:
>=20
> =EF=BB=BFOn 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 =3D=3D 1, will return
>> directly, indicates the end of the rude RCU-task grace period.
>> suppose the system has two cpus, consider the following scenario:
>>=20
>>    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 =3D=3D 1
>>       ->return
>> ....
>> task1 schedule in
>> ->free old_data0
>>                                                preempt_enable
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>>=20
>> First, good eyes and good catch!!!
>>=20
>> 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:
>>=20
>>    if (rcu_scheduler_active !=3D RCU_SCHEDULER_RUNNING &&
>>        num_online_cpus() <=3D 1)
>>        return;    // Early boot fastpath for only one CPU.
>=20
> Hi Paul
>=20
> During system startup, because the RCU_SCHEDULER_RUNNING is set after star=
ting other CPUs,=20
>=20
>              CPU0                                                         =
              CPU1                                                          =
      =20
>=20
> if (rcu_scheduler_active !=3D                                   =20
>    RCU_SCHEDULER_RUNNING &&
>           __num_online_cpus  =3D=3D 1)                                    =
          =20
>    return;                                                                =
         inc  __num_online_cpus
>                            (__num_online_cpus =3D=3D 2)
>=20
> 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 o=
peration.

If the scheduler is not yet running, then I don=E2=80=99t think missing an u=
pdate the __num_online_cpus matters since no one does a tasks-RCU synchroniz=
e.

Or did I miss something?

Thanks.


>=20
> Thanks
> Zqiang
>=20
>>=20
>> This works because rcu_scheduler_active is set to RCU_SCHEDULER_RUNNING
>> long before it is possible to offline CPUs.
>>=20
>> 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.
>>=20
>> Until further notice, anyway.
>>=20
>> 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!).
>>=20
>>                            Thanx, Paul
>>=20
>> ---
>> kernel/rcu/tasks.h | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>=20
>> 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)
>> {
>> }
>>=20
>> +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() <=3D 1)
>> -        return;    // Fastpath for only one CPU.
>> +        goto end;// Fastpath for only one CPU.
>>=20
>>    rtp->n_ipis +=3D cpumask_weight(cpu_online_mask);
>> -    schedule_on_each_cpu(rcu_tasks_be_rude);
>> +    for_each_online_cpu(cpu) {
>> +        work =3D 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();
>> }
>>=20
>> void call_rcu_tasks_rude(struct rcu_head *rhp, rcu_callback_t func);
>> --=20
>> 2.25.1
>>=20