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.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
