Re: [RFC PATCH 3/4] Replace per-subsystem mutexes with get_online_cpus

[Oleg Nesterov <oleg@tv-sign.ru>]

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.