Re: [PATCH 1/9] workqueue: perform cpu down operations from lowpriority cpu_notifier()

[Paul E. McKenney]

On Tue, Jul 17, 2012 at 10:12:21AM -0700, Tejun Heo wrote:
> Currently, all workqueue cpu hotplug operations run off
> CPU_PRI_WORKQUEUE which is higher than normal notifiers.  This is to
> ensure that workqueue is up and running while bringing up a CPU before
> other notifiers try to use workqueue on the CPU.
> 
> Per-cpu workqueues are supposed to remain working and bound to the CPU
> for normal CPU_DOWN_PREPARE notifiers.  This holds mostly true even
> with workqueue offlining running with higher priority because
> workqueue CPU_DOWN_PREPARE only creates a bound trustee thread which
> runs the per-cpu workqueue without concurrency management without
> explicitly detaching the existing workers.
> 
> However, if the trustee needs to create new workers, it creates
> unbound workers which may wander off to other CPUs while
> CPU_DOWN_PREPARE notifiers are in progress.  Furthermore, if the CPU
> down is cancelled, the per-CPU workqueue may end up with workers which
> aren't bound to the CPU.
> 
> While reliably reproducible with a convoluted artificial test-case
> involving scheduling and flushing CPU burning work items from CPU down
> notifiers, this isn't very likely to happen in the wild, and, even
> when it happens, the effects are likely to be hidden by the following
> successful CPU down.
> 
> Fix it by using different priorities for up and down notifiers - high
> priority for up operations and low priority for down operations.

Cool!!!

This certainly provides another data point in favor of running down
notifiers in the opposite order from up notifiers.  ;-)

This series passes light rcutorture/hotplug testing, will be testing
it more.

							Thanx, Paul

> Workqueue cpu hotplug operations will soon go through further cleanup.
> 
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  include/linux/cpu.h |    5 +++--
>  kernel/workqueue.c  |   38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 2e9b9eb..ce7a074 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -73,8 +73,9 @@ enum {
>  	/* migration should happen before other stuff but after perf */
>  	CPU_PRI_PERF		= 20,
>  	CPU_PRI_MIGRATION	= 10,
> -	/* prepare workqueues for other notifiers */
> -	CPU_PRI_WORKQUEUE	= 5,
> +	/* bring up workqueues before normal notifiers and down after */
> +	CPU_PRI_WORKQUEUE_UP	= 5,
> +	CPU_PRI_WORKQUEUE_DOWN	= -5,
>  };
> 
>  #define CPU_ONLINE		0x0002 /* CPU (unsigned)v is up */
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 4fa9e35..f59b7fd 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3644,6 +3644,41 @@ err_destroy:
>  	return NOTIFY_BAD;
>  }
> 
> +/*
> + * Workqueues should be brought up before normal priority CPU notifiers.
> + * This will be registered high priority CPU notifier.
> + */
> +static int __devinit workqueue_cpu_up_callback(struct notifier_block *nfb,
> +					       unsigned long action,
> +					       void *hcpu)
> +{
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_UP_PREPARE:
> +	case CPU_UP_CANCELED:
> +	case CPU_DOWN_FAILED:
> +	case CPU_ONLINE:
> +		return workqueue_cpu_callback(nfb, action, hcpu);
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +/*
> + * Workqueues should be brought down after normal priority CPU notifiers.
> + * This will be registered as low priority CPU notifier.
> + */
> +static int __devinit workqueue_cpu_down_callback(struct notifier_block *nfb,
> +						 unsigned long action,
> +						 void *hcpu)
> +{
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_DOWN_PREPARE:
> +	case CPU_DYING:
> +	case CPU_POST_DEAD:
> +		return workqueue_cpu_callback(nfb, action, hcpu);
> +	}
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef CONFIG_SMP
> 
>  struct work_for_cpu {
> @@ -3839,7 +3874,8 @@ static int __init init_workqueues(void)
>  	unsigned int cpu;
>  	int i;
> 
> -	cpu_notifier(workqueue_cpu_callback, CPU_PRI_WORKQUEUE);
> +	cpu_notifier(workqueue_cpu_up_callback, CPU_PRI_WORKQUEUE_UP);
> +	cpu_notifier(workqueue_cpu_down_callback, CPU_PRI_WORKQUEUE_DOWN);
> 
>  	/* initialize gcwqs */
>  	for_each_gcwq_cpu(cpu) {
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/