Messages in this thread | | | Date | Fri, 28 Jul 2006 13:20:41 -0500 | From | Nathan Lynch <> | Subject | Re: [PATCH -mm][resend] Disable CPU hotplug during suspend |
| |
Hi Rafael-
A couple of minor comments:
> +int cpu_down(unsigned int cpu) > +{ > + int err = 0; > + > + mutex_lock(&cpu_add_remove_lock); > + if (cpu_hotplug_disabled) > + err = -EPERM; > + else > + err = __cpu_down(cpu); > + > mutex_unlock(&cpu_add_remove_lock); > return err; > } > @@ -191,6 +203,11 @@ int __devinit cpu_up(unsigned int cpu) > void *hcpu = (void *)(long)cpu; > > mutex_lock(&cpu_add_remove_lock); > + if (cpu_hotplug_disabled) { > + ret = -EPERM; > + goto out; > + }
I think -EBUSY would be more appropriate than -EPERM, perhaps?
> +#ifdef CONFIG_SUSPEND_SMP > +static cpumask_t frozen_cpus; > + > +int disable_nonboot_cpus(void) > +{ > + int cpu, error = 0; > + > + /* We take all of the non-boot CPUs down in one shot to avoid races > + * with the userspace trying to use the CPU hotplug at the same time > + */ > + mutex_lock(&cpu_add_remove_lock); > + cpus_clear(frozen_cpus); > + printk("Disabling non-boot CPUs ...\n"); > + for_each_online_cpu(cpu) { > + if (cpu == 0) > + continue;
Assuming cpu 0 is online is not okay in generic code. This should be something like:
int cpu, first_cpu, error = 0;
/* We take all of the non-boot CPUs down in one shot to avoid races * with the userspace trying to use the CPU hotplug at the same time */ mutex_lock(&cpu_add_remove_lock); cpus_clear(frozen_cpus); first_cpu = first_cpu(cpu_online_mask); printk("Disabling non-boot CPUs ...\n"); for_each_online_cpu(cpu) { if (cpu == first_cpu) continue;
> + error = __cpu_down(cpu); > + if (!error) { > + cpu_set(cpu, frozen_cpus); > + printk("CPU%d is down\n", cpu); > + } else { > + printk(KERN_ERR "Error taking CPU%d down: %d\n", > + cpu, error); > + break; > + } > + } > + if (!error) { > + BUG_ON(num_online_cpus() > 1); > + BUG_ON(raw_smp_processor_id() != 0);
Same problem here.
Otherwise, I think the patch looks okay.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |