On 01/21/20 17:03, Russell King - ARM Linux admin wrote:
> On Mon, Nov 25, 2019 at 11:27:41AM +0000, Qais Yousef wrote:
> > +void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> > +{
> > + unsigned int cpu;
> > +
> > + if (!cpu_online(primary_cpu)) {
> > + pr_info("Attempting to shutdodwn nonboot cpus while boot cpu is offline!\n");
> > + cpu_online(primary_cpu);
Eh, that should be cpu_up(primary_cpu)!
Which I have to say I'm not if is the right thing to do.
migrate_to_reboot_cpu() picks the first online cpu if reboot_cpu (assumed 0) is
offline
migrate_to_reboot_cpu():
225 /* Make certain the cpu I'm about to reboot on is online */
226 if (!cpu_online(cpu))
227 cpu = cpumask_first(cpu_online_mask);
> > + }
> > +
> > + for_each_present_cpu(cpu) {
> > + if (cpu == primary_cpu)
> > + continue;
> > + if (cpu_online(cpu))
> > + cpu_down(cpu);
> > + }
>
> How does this avoid racing with userspace attempting to restart CPUs
> that have already been taken down by this function?
This is meant to be called from machine_shutdown() only.
But you've got a point.
The previous logic that used disable_nonboot_cpus(), which in turn called
freeze_secondary_cpus() didn't hold hotplug lock. So I assumed the higher level
logic of machine_shutdown() ensures that hotplug lock is held to synchronize
with potential other hotplug operations.
But I can see now that it doesn't.
With this series that migrates users to use device_{online,offline}, holding
the lock_device_hotplug() should protect against such races.
Worth noting that this an existing problem in the code and not something
I introduced, of course it makes sense to fix it properly as part of this
series.
I'm not sure how the other archs deal with this TBH.
Thanks for having a look!
Cheers
--
Qais Yousef
|