workqueue: break local execution guarantee of unbound work items
Subsystems (4)
Tejun Heo
maintainerPATCH 1/1 workqueue: break local execution guarantee of unbound work items
Hello,
Workqueue used to implicitly guarantee local execution of unbound work items. Recent timer updates broke that for delayed work items and the attempt to restore it ended up causing more harm than good. It has been decided to take the chance and officially break it.
This patchset reverts 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu"), expands wq_unbound_cpu_mask so that it also applies to unbound work items queued on percpu workqueues, and implements a debug feature which forces wq_unbound_cpu_mask based round-robin selection to flush out usages which depend on the local execution guarantee.
I'll push the patchset through wq/for-4.5-fixes soon.
The patchset contains the following three patches.
0001-Revert-workqueue-make-sure-delayed-work-run-in-local.patch 0002-workqueue-schedule-WORK_CPU_UNBOUND-work-on-wq_unbou.patch 0003-workqueue-implement-workqueue.debug_force_rr_cpu-deb.patch
and available in the following git branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-rr_cpu
Documentation/kernel-parameters.txt | 11 ++++++ kernel/workqueue.c | 61 ++++++++++++++++++++++++++++++++---- lib/Kconfig.debug | 15 ++++++++ 3 files changed, 81 insertions(+), 6 deletions(-)
Thanks.
--
tejun
Tejun Heo
maintainerPATCH 1/3 Revert "workqueue: make sure delayed work run in local cpu"
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, 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.
--- kernel/workqueue.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/workqueue.c +4 -4
@@ -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); }/**
Signature · 1 line
2.5.0
Re: [PATCH 1/3] Revert "workqueue: make sure delayed work run in local cpu"
Quoted 18 lines · Tejun Heo
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 by22b886dd1018("timers: Use proper base migration inadd_timer_on()"). Due to code restructuring, the commit couldn't be backported beyond certain point and stable kernels which only had874bbfe600a6started crashing.
Quoted 2 lines
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.
Quoted 6 lines
874bbfe600a6is 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.
Quoted 15 lines
Signed-off-by Tejun Heo <[email protected]>Fixes 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")Cc Mike Galbraith <[email protected]>Cc Henrique de Moraes Holschuh <[email protected]>Cc Daniel Bilik <[email protected]>Cc Jan Kara <[email protected]>Cc Shaohua Li <[email protected]>Cc Sasha Levin <[email protected]>Cc Ben Hutchings <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Daniel Bilik <[email protected]>Cc Jiri Slaby <[email protected]>Cc Michal Hocko <[email protected]>
Thanks!
Quoted 28 lines
--- kernel/workqueue.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)kernel/workqueue.c +4
-4@@ -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); }/** Signature · 1 line
2.5.0
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerPATCH 2/3 workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask CPUs
In reply to Tejun HeoFrom: Mike Galbraith <[email protected]>
WORK_CPU_UNBOUND work items queued to a bound workqueue always run locally. This is a good thing normally, but not when the user has asked us to keep unbound work away from certain CPUs. Round robin these to wq_unbound_cpumask CPUs instead, as perturbation avoidance trumps performance.
tj: Cosmetic and comment changes. WARN_ON_ONCE() dropped from empty
(wq_unbound_cpumask AND cpu_online_mask). If we want that, it should be done when config changes.
--- kernel/workqueue.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)
kernel/workqueue.c +32 -2
@@ -301,7 +301,11 @@ static DEFINE_SPINLOCK(wq_mayday_lock); /* protects wq->maydays list */static LIST_HEAD (workqueues);/* PR: list of all workqueues */ static bool workqueue_freezing;/* PL: have wqs started freezing? */ -static cpumask_var_t wq_unbound_cpumask;/* PL: low level cpumask for all unbound wqs */ +/* PL: allowable cpus for unbound wqs and work items */ +static cpumask_var_t wq_unbound_cpumask; + +/* CPU where unbound work was last round robin scheduled from this CPU */ +static DEFINE_PER_CPU(int, wq_rr_cpu_last);/* the per-cpu worker pools */ static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS],
@@ -1298,6 +1302,32 @@ static bool is_chained_work(struct workqueue_struct *wq)return worker && worker->current_pwq->wq == wq; } +/* +* When queueing an unbound work item to a wq, prefer local CPU if allowed +* by wq_unbound_cpumask. Otherwise, round robin among the allowed ones to +* avoid perturbing sensitive tasks. +*/ +static int wq_select_unbound_cpu (int cpu) +{ +int new_cpu; + +if (cpumask_test_cpu (cpu, wq_unbound_cpumask)) +return cpu; +if (cpumask_empty (wq_unbound_cpumask)) +return cpu; + + new_cpu =__this_cpu_read (wq_rr_cpu_last); + new_cpu =cpumask_next_and (new_cpu, wq_unbound_cpumask, cpu_online_mask); +if (unlikely (new_cpu >= nr_cpu_ids)) { + new_cpu =cpumask_first_and (wq_unbound_cpumask, cpu_online_mask); +if (unlikely (new_cpu >= nr_cpu_ids)) +return cpu; + } +__this_cpu_write (wq_rr_cpu_last, new_cpu); + +return new_cpu; +} +static void __queue_work (int cpu,struct workqueue_struct *wq,struct work_struct *work) {
@@ -1323,7 +1353,7 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,return ; retry:if (req_cpu ==WORK_CPU_UNBOUND ) - cpu =raw_smp_processor_id (); + cpu =wq_select_unbound_cpu (raw_smp_processor_id ());/* pwq which will be used unless @work is executing elsewhere */ if (!(wq->flags &WQ_UNBOUND ))
Signature · 1 line
2.5.0
Tejun Heo
maintainerPATCH 3/3 workqueue: implement "workqueue.debug_force_rr_cpu" debug feature
In reply to Tejun HeoWorkqueue used to guarantee local execution for work items queued without explicit target CPU. The guarantee is gone now which can break some usages in subtle ways. To flush out those cases, this patch implements a debug feature which forces round-robin CPU selection for all such work items.
The debug feature defaults to off and can be enabled with a kernel parameter. The default can be flipped with a debug config option.
If you hit this commit during bisection, please refer to 041bd12e272c ("Revert "workqueue: make sure delayed work run in local cpu"") for more information and ping me.
--- Documentation/kernel-parameters.txt | 11 +++++++++++ kernel/workqueue.c | 23 +++++++++++++++++++++-- lib/Kconfig.debug | 15 +++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-)
Documentation/kernel-parameters.txt +11 -0
@@ -4230,6 +4230,17 @@ bytes respectively. Such letter suffixes can also be entirely omitted. The default value of this parameter is determined by the config option CONFIG_WQ_POWER_EFFICIENT_DEFAULT. + workqueue.debug_force_rr_cpu + Workqueue used to implicitly guarantee that work + items queued without explicit CPU specified are put + on the local CPU. This guarantee is no longer true + and while local CPU is still preferred work items + may be put on foreign CPUs. This debug option + forces round-robin CPU selection to flush out + usages which depend on the now broken guarantee. + When enabled, memory and cache locality will be + impacted. + x2apic_phys [X86-64,APIC] Use x2apic physical mode instead of default x2apic cluster mode on platforms supporting x2apic.
kernel/workqueue.c +21 -2
@@ -307,6 +307,18 @@ static cpumask_var_t wq_unbound_cpumask;/* CPU where unbound work was last round robin scheduled from this CPU */ static DEFINE_PER_CPU(int, wq_rr_cpu_last); +/* +* Local execution of unbound work items is no longer guaranteed. The +* following always forces round-robin CPU selection on unbound work items +* to uncover usages which depend on it. +*/ +#ifdef CONFIG_DEBUG_WQ_FORCE_RR_CPU +static bool wq_debug_force_rr_cpu = true; +#else +static bool wq_debug_force_rr_cpu = false; +#endif +module_param_named (debug_force_rr_cpu, wq_debug_force_rr_cpu, bool,0644 ); +/* the per-cpu worker pools */ static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS], cpu_worker_pools);
@@ -1309,10 +1321,17 @@ static bool is_chained_work(struct workqueue_struct *wq)*/ static int wq_select_unbound_cpu (int cpu) { +static bool printed_dbg_warning;int new_cpu; -if (cpumask_test_cpu (cpu, wq_unbound_cpumask)) -return cpu; +if (likely (!wq_debug_force_rr_cpu)) { +if (cpumask_test_cpu (cpu, wq_unbound_cpumask)) +return cpu; + }else if (!printed_dbg_warning) { +pr_warn ("workqueue: round-robin CPU selection forced, expect performance impact\n" ); + printed_dbg_warning = true; + } +if (cpumask_empty (wq_unbound_cpumask))return cpu;
lib/Kconfig.debug +15 -0
@@ -1400,6 +1400,21 @@ config RCU_EQS_DEBUG endmenu # "RCU Debugging" +config DEBUG_WQ_FORCE_RR_CPU + bool "Force round-robin CPU selection for unbound work items" + depends on DEBUG_KERNEL + default n + help + Workqueue used to implicitly guarantee that work items queued + without explicit CPU specified are put on the local CPU. This + guarantee is no longer true and while local CPU is still + preferred work items may be put on foreign CPUs. Kernel + parameter "workqueue.debug_force_rr_cpu" is added to force + round-robin CPU selection to flush out usages which depend on the + now broken guarantee. This config option enables the debug + feature by default. When enabled, memory and cache locality will + be impacted. + config DEBUG_BLOCK_EXT_DEVT bool "Force extended block device numbers and spread them" depends on DEBUG_KERNEL
Signature · 1 line
2.5.0
Re: [PATCH 3/3] workqueue: implement "workqueue.debug_force_rr_cpu" debug feature
Quoted 8 lines · Tejun Heo
Workqueue used to guarantee local execution for work items queued without explicit target CPU. The guarantee is gone now which can break some usages in subtle ways. To flush out those cases, this patch implements a debug feature which forces round-robin CPU selection for all such work items.
The debug feature defaults to off and can be enabled with a kernel parameter. The default can be flipped with a debug config option.
Makes sense to me
Quoted 6 lines
If you hit this commit during bisection, please refer to
041bd12e272c("Revert "workqueue: make sure delayed work run in local cpu"") for more information and ping me.Signed-off-by Tejun Heo <[email protected]>
Thanks!
Quoted 99 lines
--- Documentation/kernel-parameters.txt | 11 +++++++++++ kernel/workqueue.c | 23 +++++++++++++++++++++-- lib/Kconfig.debug | 15 +++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-)Documentation/kernel-parameters.txt +11
-0@@ -4230,6 +4230,17 @@ bytes respectively. Such letter suffixes can also be entirely omitted. The default value of this parameter is determined by the config option CONFIG_WQ_POWER_EFFICIENT_DEFAULT. + workqueue.debug_force_rr_cpu + Workqueue used to implicitly guarantee that work + items queued without explicit CPU specified are put + on the local CPU. This guarantee is no longer true + and while local CPU is still preferred work items + may be put on foreign CPUs. This debug option + forces round-robin CPU selection to flush out + usages which depend on the now broken guarantee. + When enabled, memory and cache locality will be + impacted. + x2apic_phys [X86-64,APIC] Use x2apic physical mode instead of default x2apic cluster mode on platforms supporting x2apic.kernel/workqueue.c +21
-2@@ -307,6 +307,18 @@ static cpumask_var_t wq_unbound_cpumask;/* CPU where unbound work was last round robin scheduled from this CPU */ static DEFINE_PER_CPU(int, wq_rr_cpu_last); +/* +* Local execution of unbound work items is no longer guaranteed. The +* following always forces round-robin CPU selection on unbound work items +* to uncover usages which depend on it. +*/ +#ifdef CONFIG_DEBUG_WQ_FORCE_RR_CPU +static bool wq_debug_force_rr_cpu = true; +#else +static bool wq_debug_force_rr_cpu = false; +#endif +module_param_named (debug_force_rr_cpu, wq_debug_force_rr_cpu, bool,0644 ); +/* the per-cpu worker pools */ static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS], cpu_worker_pools);@@ -1309,10 +1321,17 @@ static bool is_chained_work(struct workqueue_struct *wq)*/ static int wq_select_unbound_cpu (int cpu) { +static bool printed_dbg_warning;int new_cpu; -if (cpumask_test_cpu (cpu, wq_unbound_cpumask)) -return cpu; +if (likely (!wq_debug_force_rr_cpu)) { +if (cpumask_test_cpu (cpu, wq_unbound_cpumask)) +return cpu; + }else if (!printed_dbg_warning) { +pr_warn ("workqueue: round-robin CPU selection forced, expect performance impact\n" ); + printed_dbg_warning = true; + } +if (cpumask_empty (wq_unbound_cpumask))return cpu;lib/Kconfig.debug +15
-0@@ -1400,6 +1400,21 @@ config RCU_EQS_DEBUG endmenu # "RCU Debugging" +config DEBUG_WQ_FORCE_RR_CPU + bool "Force round-robin CPU selection for unbound work items" + depends on DEBUG_KERNEL + default n + help + Workqueue used to implicitly guarantee that work items queued + without explicit CPU specified are put on the local CPU. This + guarantee is no longer true and while local CPU is still + preferred work items may be put on foreign CPUs. Kernel + parameter "workqueue.debug_force_rr_cpu" is added to force + round-robin CPU selection to flush out usages which depend on the + now broken guarantee. This config option enables the debug + feature by default. When enabled, memory and cache locality will + be impacted. + config DEBUG_BLOCK_EXT_DEVT bool "Force extended block device numbers and spread them" depends on DEBUG_KERNELSignature · 1 line
2.5.0
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 7 lines · Tejun Heo
This patchset reverts
874bbfe600a6("workqueue: make sure delayed work run in local cpu"), expandswq_unbound_cpu_maskso that it also applies to unbound work items queued on percpu workqueues, and implements a debug feature which forceswq_unbound_cpu_maskbased round-robin selection to flush out usages which depend on the local execution guarantee.
Looks all very reasonable to me.
Linus
Quoted 14 lines · Tejun Heo
Hello,
Workqueue used to implicitly guarantee local execution of unbound work items. Recent timer updates broke that for delayed work items and the attempt to restore it ended up causing more harm than good. It has been decided to take the chance and officially break it.
This patchset reverts
874bbfe600a6("workqueue: make sure delayed work run in local cpu"), expandswq_unbound_cpu_maskso that it also applies to unbound work items queued on percpu workqueues, and implements a debug feature which forceswq_unbound_cpu_maskbased round-robin selection to flush out usages which depend on the local execution guarantee.
Quoted 7 lines
I'll push the patchset through wq/for-4.5-fixes soon.
The patchset contains the following three patches.
0001-Revert-workqueue-make-sure-delayed-work-run-in-local.patch 0002-workqueue-schedule-WORK_CPU_UNBOUND-work-on-wq_unbou.patch 0003-workqueue-implement-workqueue.debug_force_rr_cpu-deb.patch
Thanks all for sorting the issue out. Now it remains to decide what should go to stable. Given only 0001 is marked as "Fixes", is that enough?
And what about the vmstat fix:
commit 176bed1de5bf977938cad26551969eca8f0883b1
Author: Linus Torvalds <[email protected]> Date: Thu Oct 15 13:01:50 2015 -0700
vmstat: explicitly schedule per-cpu work on the CPU we need it to run on
? It should fix better than what 0001 is reverting AFAIU, right?
thanks,
Signature · 2 lines
js suse labs
Tejun Heo
maintainerHello, Jiri.
Quoted 2 lines · Jiri Slaby
Thanks all for sorting the issue out. Now it remains to decide what should go to stable. Given only 0001 is marked as "Fixes", is that enough?
I think so. The other two are primarily for debugging anyway.
Quoted 8 lines
And what about the vmstat fix:
commit176bed1de5bf977938cad26551969eca8f0883b1Author: Linus Torvalds <[email protected]> Date: Thu Oct 15 13:01:50 2015 -0700vmstat: explicitly schedule per-cpu work on the CPU we need it to run on
? It should fix better than what 0001 is reverting AFAIU, right?
Yes, definitely. Didn't know that one wasn't tagged for stable.
Thanks.
Signature · 1 line
tejun