Re: [RFC PATCH v2 01/10] CPU hotplug: Provide APIs for "light" atomic readers to prevent CPU offline
From: Oleg Nesterov <oleg@redhat.com>
Date: 2012-12-05 19:07:28
Also in:
lkml
I'll try to read this series later, one minor and almost offtopic nit. On 12/06, Srivatsa S. Bhat wrote:
static int __ref take_cpu_down(void *_param)
{
struct take_cpu_down_param *param = _param;
+ unsigned long flags;
int err;
+ /*
+ * __cpu_disable() is the step where the CPU is removed from the
+ * cpu_online_mask. Protect it with the light-lock held for write.
+ */
+ write_lock_irqsave(&light_hotplug_rwlock, flags);
+
/* Ensure this CPU doesn't handle any more interrupts. */
err = __cpu_disable();
- if (err < 0)
+ if (err < 0) {
+ write_unlock_irqrestore(&light_hotplug_rwlock, flags);
return err;
+ }
+
+ /*
+ * We have successfully removed the CPU from the cpu_online_mask.
+ * So release the light-lock, so that the light-weight atomic readers
+ * (who care only about the cpu_online_mask updates, and not really
+ * about the actual cpu-take-down operation) can continue.
+ *
+ * But don't enable interrupts yet, because we still have work left to
+ * do, to actually bring the CPU down.
+ */
+ write_unlock(&light_hotplug_rwlock);
cpu_notify(CPU_DYING | param->mod, param->hcpu);
+
+ local_irq_restore(flags);
return 0;This is subjective, but imho _irqsave and the fat comment look confusing. Currently take_cpu_down() is always called with irqs disabled, so you do not need to play with interrupts. 10/10 does s/__stop_machine/stop_cpus/ and that patch could simply add local_irq_disable/enable into take_cpu_down(). But again this is minor and subjective, I won't insist. Oleg.