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 18:30:19 -0500
Message-Id: <0E3DA71D-8C94-4FE9-BBF9-83AC28993485@joelfernandes.org>
References: <20221129201349.GE4001@paulmck-ThinkPad-P17-Gen-1>
Cc:     "Zhang, Qiang1" <qiang1.zhang@intel.com>, frederic@kernel.org,
        quic_neeraju@quicinc.com, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
In-Reply-To: <20221129201349.GE4001@paulmck-ThinkPad-P17-Gen-1>
To:     paulmck@kernel.org
Precedence: bulk


> On Nov 29, 2022, at 3:13 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
>=20
> =EF=BB=BFOn Tue, Nov 29, 2022 at 03:01:12PM -0500, Joel Fernandes wrote:
>>>> On Nov 29, 2022, at 2:18 PM, Paul E. McKenney <paulmck@kernel.org> wrot=
e:
>>> =EF=BB=BFOn Tue, Nov 29, 2022 at 11:00:05AM -0500, Joel Fernandes wrote:=

>>>>>> On Nov 29, 2022, at 10:18 AM, Paul E. McKenney <paulmck@kernel.org> w=
rote:
>>>>> =EF=BB=BFOn Tue, Nov 29, 2022 at 06:25:04AM +0000, Zhang, Qiang1 wrote=
:
>>>>>>>> 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 bo=
ot.
>>>>>>>> So a simpler way to make that work is to check for RCU_SCHEDULER_RU=
NNING,
>>>>>>>> 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 afte=
r starting 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 C=
PU1 in time
>>>>>>> Can we move rcu_set_runtime_mode() before smp_init()
>>>>>>> any thoughts?
>>>>>>>=20
>>>>>>> Is anyone expected to do rcu-tasks operation before the scheduler is=
 running?=20
>>>>>>=20
>>>>>> Not sure if such a scenario exists.
>>>>>>=20
>>>>>>> Typically this requires the tasks to context switch which is a sched=
uler operation.
>>>>>>>=20
>>>>>>> If the scheduler is not yet running, then I don=E2=80=99t think miss=
ing an update the __num_online_cpus matters since no one does a tasks-RCU sy=
nchronize.
>>>>>>=20
>>>>>> Hi Joel
>>>>>>=20
>>>>>> After the kernel_init task runs, before calling smp_init() to startin=
g other CPUs,=20
>>>>>> the scheduler haven been initialization, task context switching can o=
ccur.
>>>>>=20
>>>>> Good catch, thank you both.  For some reason, I was thinking that the
>>>>> additional CPUs did not come online until later.
>>>>>=20
>>>>> So how about this?
>>>>>=20
>>>>>  if (rcu_scheduler_active =3D=3D RCU_SCHEDULER_INACTIVE)
>>>>>      return;    // Early boot fastpath.
>>>>>=20
>>>>> If this condition is true, there is only one CPU and no scheduler,
>>>>> thus no preemption.
>>>>=20
>>>> Agreed. I was going to suggest exactly this :)
>>>>=20
>>>> Ack.
>>>> (Replying by phone but feel free to add my reviewed by tag).
>>>=20
>>> I should add that the downside of this approach is that there is a short=

>>> time between the scheduler initializing and workqueues fully initializin=
g
>>> 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.=

>>>=20
>>> So this should be fine, but please recall this for when/if someone does
>>> stick a synchronize_rcu_tasks() into that short time.  ;-)
>>=20
>> Thanks Paul, but why would anyone want to do sync rcu tasks, before
>> the scheduler is fully initialized?
>=20
> I could ask that same question of a number of other RCU API members.  ;-)
>=20
>> 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.
>=20
> 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=20

>=20
>                            Thanx, Paul
>=20
>> Thanks,
>>=20
>> - Joel
>>=20
>>>=20
>>>                           Thanx, Paul
>>>=20
>>>> - Joel
>>>>=20
>>>>=20
>>>>>=20
>>>>>                      Thanx, Paul
>>>>>=20
>>>>>> Thanks
>>>>>> Zqiang
>>>>>>=20
>>>>>>>=20
>>>>>>> Or did I miss something?
>>>>>>>=20
>>>>>>> Thanks.
>>>>>>>=20
>>>>>>>=20
>>>>>>>=20
>>>>>>>=20
>>>>>>> Thanks
>>>>>>> Zqiang
>>>>>>>=20
>>>>>>>>=20
>>>>>>>> This works because rcu_scheduler_active is set to RCU_SCHEDULER_RUN=
NING
>>>>>>>> long before it is possible to offline CPUs.
>>>>>>>>=20
>>>>>>>> Yes, schedule_on_each_cpu() does do cpus_read_lock(), again, good e=
yes,
>>>>>>>> and it also unnecessarily does the schedule_work_on() the current C=
PU,
>>>>>>>> but the code calling synchronize_rcu_tasks_rude() is on high-overhe=
ad
>>>>>>>> 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_s=
truct *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