Messages in this thread | | | Date | Mon, 22 Oct 2007 10:28:18 +0530 | From | Gautham R Shenoy <> | Subject | Re: [RFC PATCH 3/4] Replace per-subsystem mutexes with get_online_cpus |
| |
On Sun, Oct 21, 2007 at 03:39:17PM +0400, Oleg Nesterov wrote: > On 10/16, Gautham R Shenoy wrote: > > > > This patch converts the known per-subsystem cpu_hotplug mutexes to > > get_online_cpus put_online_cpus. > > It also eliminates the CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE hotplug > > notification events. > > Personally, I like the changes in workqueue.c very much, a couple of > minor nits below. > > > --- linux-2.6.23.orig/kernel/workqueue.c > > +++ linux-2.6.23/kernel/workqueue.c > > @@ -592,8 +592,6 @@ EXPORT_SYMBOL(schedule_delayed_work_on); > > * Returns zero on success. > > * Returns -ve errno on failure. > > * > > - * Appears to be racy against CPU hotplug. > > - * > > see below, > > > * schedule_on_each_cpu() is very slow. > > */ > > int schedule_on_each_cpu(work_func_t func) > > @@ -605,7 +603,7 @@ int schedule_on_each_cpu(work_func_t fun > > if (!works) > > return -ENOMEM; > > > > - preempt_disable(); /* CPU hotplug */ > > + get_online_cpus(); /* CPU hotplug */ > > for_each_online_cpu(cpu) { > > struct work_struct *work = per_cpu_ptr(works, cpu); > > > > @@ -613,7 +611,7 @@ int schedule_on_each_cpu(work_func_t fun > > set_bit(WORK_STRUCT_PENDING, work_data_bits(work)); > > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), work); > > } > > - preempt_enable(); > > + put_online_cpus(); > > flush_workqueue(keventd_wq); > > Still racy. To kill the race, please move flush_workqueue() up, before > put_online_cpus(). > > > @@ -809,6 +809,7 @@ void destroy_workqueue(struct workqueue_ > > struct cpu_workqueue_struct *cwq; > > int cpu; > > > > + get_online_cpus(); > > mutex_lock(&workqueue_mutex); > > list_del(&wq->list); > > mutex_unlock(&workqueue_mutex); > > @@ -817,6 +818,7 @@ void destroy_workqueue(struct workqueue_ > > cwq = per_cpu_ptr(wq->cpu_wq, cpu); > > cleanup_workqueue_thread(cwq, cpu); > > } > > + put_online_cpus(); > > Correct, but I'd suggest to do put_online_cpus() earlier, right after > mutex_unlock(&workqueue_mutex). > > > @@ -830,22 +832,17 @@ static int __devinit workqueue_cpu_callb > > unsigned int cpu = (unsigned long)hcpu; > > struct cpu_workqueue_struct *cwq; > > struct workqueue_struct *wq; > > + int ret = NOTIFY_OK; > > > > action &= ~CPU_TASKS_FROZEN; > > > > switch (action) { > > - case CPU_LOCK_ACQUIRE: > > - mutex_lock(&workqueue_mutex); > > - return NOTIFY_OK; > > - > > - case CPU_LOCK_RELEASE: > > - mutex_unlock(&workqueue_mutex); > > - return NOTIFY_OK; > > > > please remove this emtpy line > > > case CPU_UP_PREPARE: > > cpu_set(cpu, cpu_populated_map); > > } > > > > + mutex_lock(&workqueue_mutex); > > We don't need workqueue_mutex here. With your patch workqueue_mutex protects > list_head, nothing more. But all other callers (create/destroy) should take > get_online_cpus() anyway. This means that we can convert workqueue_mutex to > spinlock_t.
Thanks for the review! Will code these changes up in the next version and post them sometime soon.
> > Oleg. >
Thanks and Regards gautham. -- Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!" - 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/
| |