Skip to contentSkip to conversation

mod_delayed_work() explosion due to 874bbfe6

PATCH In review Mike Galbraith 4 participants 15 messages Feb 2, 2016 – Feb 15, 2016 Workqueue

lore
Share & tools
lore
#0 lore raw

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 ffff8c03fdf0e328
ffff8c03fdf0e328: 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

#1 lore raw
Quoted 21 lines · Mike Galbraith

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 ffff8c03fdf0e328
ffff8c03fdf0e328: 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
maintainer
#2 lore raw
In reply to Mike Galbraith
Quoted 16 lines · Mike Galbraith

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 ffff8c03fdf0e328
ffff8c03fdf0e328: 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
maintainer
#3 lore raw

PATCH 1/1 workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup

In reply to Mike Galbraith

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 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.

Cc Tang Chen <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
Fixes 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
---
 kernel/workqueue.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
kernel/workqueue.c +10 -0
index 61a0264..f748eab 100644
@@ -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]);
 }
 
maintainer
#4 lore raw

Re: [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_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.

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
#5 lore raw

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_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.

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

#6 lore raw

Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup

In reply to Tejun Heo
Quoted 20 lines · Tejun Heo

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 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.

Quoted 6 lines

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
index 61a0264..f748eab 100644

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

maintainer
#7 lore raw

Re: [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

4.3+ ? Hasn't 874bbfe600a6 been 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
#8 lore raw

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

4.3+ ? Hasn't 874bbfe600a6 been 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_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.

That may make confirming the fix a bit problematic. The crash I was looking at happened on a Fujitsu box.

-Mike

Reviewed-by on v1 1/1
#9 lore raw

Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup

In reply to Tejun Heo
Quoted 20 lines · Tejun Heo

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 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.

Quoted 9 lines
Cc Tang Chen <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
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
index 61a0264..f748eab 100644
@@ -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
maintainer
#10 lore raw

PATCH 1/1 workqueue: handle NUMA_NO_NODE for unbound pool_workqueue lookup

In reply to Tejun Heo

Hello,

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 9354baa208f47bf088c28c1da5f38abe172db5d6 Mon 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
index dc7faad..3ca7ab8 100644
@@ -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
#11 lore raw

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
index c579dbab2e36..4785b895b9b5 100644
@@ -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
maintainer
#12 lore raw

Re: [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_mem to 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
#13 lore raw

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_mem to 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
maintainer
#14 lore raw

Re: [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_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?

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