Re: [PATCH 1/3] Revert "workqueue: make sure delayed work run in local cpu"

[Michal Hocko]

On Tue 09-02-16 18:14:48, Tejun Heo wrote:
> This reverts commit 874bbfe600a660cba9c776b3957b1ce393151b76.
> 
> Workqueue used to implicity guarantee that work items queued without
> explicit CPU specified are put on the local CPU.  Recent changes in
> timer broke the guarantee and led to vmstat breakage which was fixed
> by 176bed1de5bf ("vmstat: explicitly schedule per-cpu work on the CPU
> we need it to run on").
> 
> vmstat is the most likely to expose the issue and it's quite possible
> that there are other similar problems which are a lot more difficult
> to trigger.  As a preventive measure, 874bbfe600a6 ("workqueue: make
> sure delayed work run in local cpu") was applied to restore the local
> CPU guarnatee.  Unfortunately, the change exposed a bug in timer code
> which got fixed by 22b886dd1018 ("timers: Use proper base migration in
> add_timer_on()").  Due to code restructuring, the commit couldn't be
> backported beyond certain point and stable kernels which only had
> 874bbfe600a6 started crashing.
> 
> The local CPU guarantee was accidental more than anything else and we
> want to get rid of it anyway.  As, with the vmstat case fixed,

I would find it helpful to mention 176bed1de5bf ("vmstat: explicitly
schedule per-cpu work on the CPU we need it to run on") as the vmstat
fix.

> 874bbfe600a6 is causing more problems than it's fixing, it has been
> decided to take the chance and officially break the guarantee by
> reverting the commit.  A debug feature will be added to force foreign
> CPU assignment to expose cases relying on the guarantee and fixes for
> the individual cases will be backported to stable as necessary.
> 
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Fixes: 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
> Link: http://lkml.kernel.org/g/20160120211926.GJ10810@xxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Mike Galbraith <umgwanakikbuti@xxxxxxxxx>
> Cc: Henrique de Moraes Holschuh <hmh@xxxxxxxxxx>
> Cc: Daniel Bilik <daniel.bilik@xxxxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Shaohua Li <shli@xxxxxx>
> Cc: Sasha Levin <sasha.levin@xxxxxxxxxx>
> Cc: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Daniel Bilik <daniel.bilik@xxxxxxxxxxxx>
> Cc: Jiri Slaby <jslaby@xxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks!

> ---
>  kernel/workqueue.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index dc7faad..5e63d3b 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1464,13 +1464,13 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
>  	timer_stats_timer_set_start_info(&dwork->timer);
>  
>  	dwork->wq = wq;
> -	/* timer isn't guaranteed to run in this cpu, record earlier */
> -	if (cpu == WORK_CPU_UNBOUND)
> -		cpu = raw_smp_processor_id();
>  	dwork->cpu = cpu;
>  	timer->expires = jiffies + delay;
>  
> -	add_timer_on(timer, cpu);
> +	if (unlikely(cpu != WORK_CPU_UNBOUND))
> +		add_timer_on(timer, cpu);
> +	else
> +		add_timer(timer);
>  }
>  
>  /**
> -- 
> 2.5.0

-- 
Michal Hocko
SUSE Labs