mod_delayed_work() explosion due to 874bbfe6
Hi Tejun,
I'm looking at a crash analysis that fingers 874bbfe6 as being the culprit. (I didn't do the analysis, but it appears to be correct)
Scenario: CPU168 calls mod_delayed_work(), is taken offline before the timer expires. Due to 874bbfe6, dwork->cpu is the now offline CPU168 vs the previous WORK_CPU_UNBOUND, timer fires on CPU131, it tries to __queue_work() with cpu == the now offline CPU168, gets to...
} else
pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));... and goes boom.
<snippet>
crash> p numa_node | grep 168
[168]: ffff8c03fdf0e328
crash> rd ffff8c03fdf0e328ffff8c03fdf0e328: 00000000ffffffff ........
Thus, pwq becomes 000000000. Then, as the result of reference to pwq->pool, NULL reference occurs at (*PANIC). </snippet>
What if anything is supposed to prevent this?
-Mike
Quoted 21 lines · Mike Galbraith
Hi Tejun,
I'm looking at a crash analysis that fingers
874bbfe6as being the culprit. (I didn't do the analysis, but it appears to be correct)Scenario: CPU168 calls
mod_delayed_work(), is taken offline before the timer expires. Due to874bbfe6,dwork->cpuis the now offline CPU168 vs the previousWORK_CPU_UNBOUND, timer fires on CPU131, it tries to__queue_work()with cpu == the now offline CPU168, gets to...} else pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));... and goes boom.
<snippet>
crash> pnuma_node| grep 168
[168]:ffff8c03fdf0e328
crash> rdffff8c03fdf0e328ffff8c03fdf0e328:00000000ffffffff........
Quoted 5 lines
Thus, pwq becomes 000000000. Then, as the result of reference to
pwq->pool, NULL reference occurs at (*PANIC). </snippet>What if anything is supposed to prevent this?
AFAICS [1] nothing...
---
[1] http://lkml.kernel.org/r/[email protected]
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerQuoted 16 lines · Mike Galbraith
Scenario: CPU168 calls
mod_delayed_work(), is taken offline before the timer expires. Due to874bbfe6,dwork->cpuis the now offline CPU168 vs the previousWORK_CPU_UNBOUND, timer fires on CPU131, it tries to__queue_work()with cpu == the now offline CPU168, gets to...} else pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));... and goes boom.
<snippet>
crash> pnuma_node| grep 168
[168]:ffff8c03fdf0e328
crash> rdffff8c03fdf0e328ffff8c03fdf0e328:00000000ffffffff........
Quoted 5 lines
Thus, pwq becomes 000000000. Then, as the result of reference to
pwq->pool, NULL reference occurs at (*PANIC). </snippet>What if anything is supposed to prevent this?
(cc'ing
So, the root problem here is cpu <-> node mapping flipping across cpu on/offlining which is unnecessary and causes other issues too. It's being worked on for quite a while now.
http://lkml.kernel.org/g/[email protected]
I'll create a bandaid patch for now so that wq falls back to dfl_wq on negative node.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 1/1 workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
In reply to Mike GalbraithWhen looking up the pool_workqueue to use for an unbound workqueue, workqueue assumes that the target CPU is always bound to a valid NUMA node. However, currently, when a CPU goes offline, the mapping is destroyed and cpu_to_node() returns NUMA_NO_NODE. This has always been broken but hasn't triggered until recently.
After 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu"), workqueue forcifully assigns the local CPU for delayed work items without explicit target CPU to fix a different issue. This widens the window where CPU can go offline while a delayed work item is pending causing delayed work items dispatched with target CPU set to an already offlined CPU. The resulting NUMA_NO_NODE mapping makes workqueue try to queue the work item on a NULL pool_workqueue and thus crash.
Fix it by mapping NUMA_NO_NODE to the default pool_workqueue from unbound_pwq_by_node(). This is a temporary workaround. The long term solution is keeping CPU -> NODE mapping stable across CPU off/online cycles which is in the works.
--- kernel/workqueue.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
kernel/workqueue.c +10 -0
@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. +*/ +if (unlikely (node ==NUMA_NO_NODE )) +return wq->dfl_pwq; +return rcu_dereference_raw (wq->numa_pwq_tbl[node]); }
Tejun Heo
maintainerRe: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Quoted 4 lines · Tejun Heo
Fix it by mapping
NUMA_NO_NODEto the defaultpool_workqueuefromunbound_pwq_by_node(). This is a temporary workaround. The long term solution is keeping CPU -> NODE mapping stable across CPU off/online cycles which is in the works.
Forgot to mention. Can you please test this? Once verified, I'll route it through wq/for-4.5-fixes.
Thanks.
Signature · 1 line
tejun
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Quoted 10 lines · Tejun Heo
Quoted 6 lines · Tejun Heo
Fix it by mapping
NUMA_NO_NODEto the defaultpool_workqueuefromunbound_pwq_by_node(). This is a temporary workaround. The long term
solution is keeping CPU -> NODE mapping stable across CPU off/online
cycles which is in the works.Forgot to mention. Can you please test this? Once verified, I'll route it through wq/for-4.5-fixes.
I've passed it on, hopefully the user can make time to test.
-Mike
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
In reply to Tejun HeoQuoted 20 lines · Tejun Heo
When looking up the
pool_workqueueto use for an unbound workqueue, workqueue assumes that the target CPU is always bound to a valid NUMA node. However, currently, when a CPU goes offline, the mapping is destroyed andcpu_to_node()returnsNUMA_NO_NODE. This has always been broken but hasn't triggered until recently.After
874bbfe600a6("workqueue: make sure delayed work run in local cpu"), workqueue forcifully assigns the local CPU for delayed work items without explicit target CPU to fix a different issue. This widens the window where CPU can go offline while a delayed work item is pending causing delayed work items dispatched with target CPU set to an already offlined CPU. The resultingNUMA_NO_NODEmapping makes workqueue try to queue the work item on a NULLpool_workqueueand thus crash.Fix it by mapping
NUMA_NO_NODEto the defaultpool_workqueuefromunbound_pwq_by_node(). This is a temporary workaround. The long term solution is keeping CPU -> NODE mapping stable across CPU off/online cycles which is in the works.
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Mike Galbraith <[email protected]>Cc Tang Chen <[email protected]>Cc Rafael J. Wysocki <[email protected]>Cc Len Brown <[email protected]>Cc [email protected] # v4.3+
4.3+ ? Hasn't 874bbfe600a6 been backported to older stable kernels?
Adding a 'Fixes: 874bbfe600a6 ...' tag is what you really want here.
Quoted 5 lines
kernel/workqueue.c +0
-0
kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 9 lines
kernel/workqueue.c +6
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed.
So what happens if the complete node is offline?
kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 582
Quoted 4 lines
kernel/workqueue.c +4
-0+*/ +if (unlikely (node ==NUMA_NO_NODE )) +return wq->dfl_pwq; +
Thanks,
tglx
Tejun Heo
maintainerRe: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Hello,
Quoted 10 lines · Thomas Gleixner
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Mike Galbraith <[email protected]>Cc Tang Chen <[email protected]>Cc Rafael J. Wysocki <[email protected]>Cc Len Brown <[email protected]>Cc [email protected] # v4.3+4.3+ ? Hasn't
874bbfe600a6been backported to older stable kernels?Adding a 'Fixes:
874bbfe600a6...' tag is what you really want here.
Oops, you're right. Will add that once Mike confirms the fix.
Quoted 4 lines
Quoted 4 lines
kernel/workqueue.c +0
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq);
Quoted 8 lines
kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 6 lines
kernel/workqueue.c +6
-0+ +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. So what happens if the complete node is offline?
pool_workqueue lookup itself should be fine as dfl_pwq is assigned to all nodes by default. When the node comes back online, things can break currently because cpu to node mapping may change. That's what Tang has been working on. It's a bigger problem throughout the memory allocation path tho because there's no synchronization around cpu -> node mapping. Hopefully, the pending patchset can get through sooner than later.
Thanks.
Signature · 1 line
tejun
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Quoted 29 lines · Tejun Heo
Hello,
Quoted 10 lines · Thomas Gleixner
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Mike Galbraith <[email protected]>Cc Tang Chen <[email protected]>Cc Rafael J. Wysocki <[email protected]>Cc Len Brown <[email protected]>Cc [email protected] # v4.3+4.3+ ? Hasn't
874bbfe600a6been backported to older stable kernels?Adding a 'Fixes:
874bbfe600a6...' tag is what you really want here.Oops, you're right. Will add that once Mike confirms the fix.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, > > > > > > > > > > > > > > > > > > > > > int node) { > > > > > > assert_rcu_or_wq_mutex_or_pool_mutex(wq);Quoted 8 lines
Review static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 6 lines
(quoted diff) +6
-0+ +> > > > > > /* +> > > > > > * XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +> > > > > > * delayed item is pending. The plan is to keep CPU -> NODE +> > > > > > * mapping valid and stable across CPU on/offlines. Once that +> > > > > > * happens, this workaround can be removed.So what happens if the complete node is offline?
Quoted 4 lines
pool_workqueuelookup itself should be fine asdfl_pwqis assigned to all nodes by default. When the node comes back online, things can break currently because cpu to node mapping may change. That's what Tang has been working on.
That may make confirming the fix a bit problematic. The crash I was looking at happened on a Fujitsu box.
-Mike
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
In reply to Tejun HeoQuoted 20 lines · Tejun Heo
When looking up the
pool_workqueueto use for an unbound workqueue, workqueue assumes that the target CPU is always bound to a valid NUMA node. However, currently, when a CPU goes offline, the mapping is destroyed andcpu_to_node()returnsNUMA_NO_NODE. This has always been broken but hasn't triggered until recently.After
874bbfe600a6("workqueue: make sure delayed work run in local cpu"), workqueue forcifully assigns the local CPU for delayed work items without explicit target CPU to fix a different issue. This widens the window where CPU can go offline while a delayed work item is pending causing delayed work items dispatched with target CPU set to an already offlined CPU. The resultingNUMA_NO_NODEmapping makes workqueue try to queue the work item on a NULLpool_workqueueand thus crash.Fix it by mapping
NUMA_NO_NODEto the defaultpool_workqueuefromunbound_pwq_by_node(). This is a temporary workaround. The long term solution is keeping CPU -> NODE mapping stable across CPU off/online cycles which is in the works.
Quoted 9 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Mike Galbraith <[email protected]>Cc Tang Chen <[email protected]>Cc Rafael J. Wysocki <[email protected]>Cc Len Brown <[email protected]>Cc [email protected] # v4.3+Fixes 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
Quoted 25 lines
--- kernel/workqueue.c | 10 ++++++++++ 1 file changed, 10 insertions(+)kernel/workqueue.c +10
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. +*/ +if (unlikely (node ==NUMA_NO_NODE )) +return wq->dfl_pwq; +return rcu_dereference_raw (wq->numa_pwq_tbl[node]); }
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerPATCH 1/1 workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
In reply to Tejun HeoHello,
With 874bbfe600a6 reverted, this doesn't matter as much but I'm still committing it to wq/for-4.5-fixes. The commit message has been updated accordingly.
Thanks.
------ 8< ------
Quoted 1 line
From
9354baa208f47bf088c28c1da5f38abe172db5d6Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Wed, 3 Feb 2016 13:54:25 -0500
When looking up the pool_workqueue to use for an unbound workqueue, workqueue assumes that the target CPU is always bound to a valid NUMA node. However, currently, when a CPU goes offline, the mapping is destroyed and cpu_to_node() returns NUMA_NO_NODE.
This has always been broken but hasn't triggered often enough before 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu"). After the commit, workqueue forcifully assigns the local CPU for delayed work items without explicit target CPU to fix a different issue. This widens the window where CPU can go offline while a delayed work item is pending causing delayed work items dispatched with target CPU set to an already offlined CPU. The resulting NUMA_NO_NODE mapping makes workqueue try to queue the work item on a NULL pool_workqueue and thus crash.
While 874bbfe600a6 has been reverted for a different reason making the bug less visible again, it can still happen. Fix it by mapping NUMA_NO_NODE to the default pool_workqueue from unbound_pwq_by_node(). This is a temporary workaround. The long term solution is keeping CPU -> NODE mapping stable across CPU off/online cycles which is being worked on.
--- kernel/workqueue.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
kernel/workqueue.c +10 -0
@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. +*/ +if (unlikely (node ==NUMA_NO_NODE )) +return wq->dfl_pwq; +return rcu_dereference_raw (wq->numa_pwq_tbl[node]); }
Signature · 1 line
2.5.0
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
On Wed 10-02-16 10:55:03, Tejun Heo wrote:
[...]
kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 12 lines
kernel/workqueue.c +6
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed.
I am not sure this is completely true with the code as is currently. Don't wee also need to use cpu_to_mem to handle memoryless CPUs?
kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 584
Quoted 6 lines
kernel/workqueue.c +4
-0+*/ +if (unlikely (node ==NUMA_NO_NODE )) +return wq->dfl_pwq; +return rcu_dereference_raw (wq->numa_pwq_tbl[node]); }
kernel/workqueue.c +1 -1
@@ -1325,7 +1325,7 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,if (!(wq->flags &WQ_UNBOUND )) pwq =per_cpu_ptr (wq->cpu_pwqs, cpu);else - pwq =unbound_pwq_by_node (wq,cpu_to_node (cpu)); + pwq =unbound_pwq_by_node (wq,cpu_to_mem (cpu));/* * If @work was previously on a different pool, it might still be
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerRe: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Hello, Michal.
Quoted 17 lines · Michal Hocko
On Wed 10-02-16 10:55:03, Tejun Heo wrote:
[...]kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 12 lines
kernel/workqueue.c +6
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. I am not sure this is completely true with the code as is currently. Don't wee also need to use
cpu_to_memto handle memoryless CPUs?
I'm not sure. I think we still wan to distinguish workers for a memoryless node from its neighboring node with memory. We don't want work items for the latter to be randomly distributed to the former after all.
Thanks.
Signature · 1 line
tejun
Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Quoted 21 lines · Tejun Heo
Hello, Michal.
Quoted 17 lines · Michal Hocko
On Wed 10-02-16 10:55:03, Tejun Heo wrote:
[...]kernel/workqueue.c static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, · line 578
Quoted 12 lines
kernel/workqueue.c +6
-0@@ -570,6 +570,16 @@ static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq, int node) {assert_rcu_or_wq_mutex_or_pool_mutex (wq); + +/* +* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a +* delayed item is pending. The plan is to keep CPU -> NODE +* mapping valid and stable across CPU on/offlines. Once that +* happens, this workaround can be removed. I am not sure this is completely true with the code as is currently. Don't wee also need to use
cpu_to_memto handle memoryless CPUs?
Quoted 4 lines
I'm not sure. I think we still wan to distinguish workers for a memoryless node from its neighboring node with memory. We don't want work items for the latter to be randomly distributed to the former after all.
I am not sure I understand. Does that mean that a node with no memory would have its WQ specific pool? I might be missing something but I thought that cpu_to_node will return NUMA_NO_NODE if the CPU is memory less. Or do you expect that cpu_to_node will always return a valid node id even when it doesn't contain any memory?
Signature · 2 lines
Michal Hocko SUSE Labs
Tejun Heo
maintainerRe: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup
Hello, Michal.
Quoted 2 lines · Michal Hocko
I am not sure I understand. Does that mean that a node with no memory would have its WQ specific pool? I might be missing something but I
Yeah, I think it should.
Quoted 3 lines
thought that
cpu_to_nodewill returnNUMA_NO_NODEif the CPU is memory less. Or do you expect thatcpu_to_nodewill always return a valid node id even when it doesn't contain any memory?
Hmmm... that was what I was expecting. It's not really looking for memory node to use but identify affinity there.
Thanks.
Signature · 1 line
tejun