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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160215131441.GB26201@dhcp22.suse.cz>
Date:	Mon, 15 Feb 2016 14:14:41 +0100
From:	Michal Hocko <mhocko@...nel.org>
To:	Tejun Heo <tj@...nel.org>
Cc:	torvalds@...ux-foundation.org, umgwanakikbuti@...il.com,
	jslaby@...e.cz, tglx@...utronix.de, pmladek@...e.com, jack@...e.cz,
	ben@...adent.org.uk, sasha.levin@...cle.com, shli@...com,
	daniel.bilik@...system.cz, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, kernel-team@...com,
	stable@...r.kernel.org,
	Henrique de Moraes Holschuh <hmh@....eng.br>
Subject: Re: [PATCH 1/3] Revert "workqueue: make sure delayed work run in
 local cpu"

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@...nel.org>
> Fixes: 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
> Link: http://lkml.kernel.org/g/20160120211926.GJ10810@quack.suse.cz
> Cc: stable@...r.kernel.org
> Cc: Mike Galbraith <umgwanakikbuti@...il.com>
> Cc: Henrique de Moraes Holschuh <hmh@....eng.br>
> Cc: Daniel Bilik <daniel.bilik@...system.cz>
> Cc: Jan Kara <jack@...e.cz>
> Cc: Shaohua Li <shli@...com>
> Cc: Sasha Levin <sasha.levin@...cle.com>
> Cc: Ben Hutchings <ben@...adent.org.uk>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Daniel Bilik <daniel.bilik@...system.cz>
> Cc: Jiri Slaby <jslaby@...e.cz>
> Cc: Michal Hocko <mhocko@...nel.org>

Acked-by: Michal Hocko <mhocko@...e.com>

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