Messages in this thread | | | From | Joel Fernandes <> | Subject | Re: [PATCH v2] rcu-tasks: Make rude RCU-Tasks work well with CPU hotplug | Date | Tue, 29 Nov 2022 15:01:12 -0500 |
| |
> On Nov 29, 2022, at 2:18 PM, Paul E. McKenney <paulmck@kernel.org> 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@kernel.org> 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@intel.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@intel.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? >>>> >>>> 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? 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.
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 >>>>>>
| |