Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1324146 > unrolled thread

mod_delayed_work() explosion due to 874bbfe6

Back to article view | Back to linux.kernel

Contents

  mod_delayed_work()  explosion due to  874bbfe6 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-02 15:50 +0100
    Re: mod_delayed_work()  explosion due to  874bbfe6 Michal Hocko <mhocko@kernel.org> - 2016-02-03 15:40 +0100
    Re: mod_delayed_work()  explosion due to  874bbfe6 Tejun Heo <tj@kernel.org> - 2016-02-03 17:40 +0100
    [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for unbound  pool_workqueue lookup Tejun Heo <tj@kernel.org> - 2016-02-03 20:00 +0100
      Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Tejun Heo <tj@kernel.org> - 2016-02-03 20:00 +0100
        Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-04 04:20 +0100
      Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Thomas Gleixner <tglx@linutronix.de> - 2016-02-03 20:20 +0100
        Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Tejun Heo <tj@kernel.org> - 2016-02-03 20:30 +0100
          Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-04 03:20 +0100
      Re: [PATCH wq/for-4.5-fixes] workqueue: handle NUMA_NO_NODE for  unbound pool_workqueue lookup Michal Hocko <mhocko@kernel.org> - 2016-02-04 09:50 +0100

#1324146 — 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 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

[toc] | [next] | [standalone]

#1325499

On Tue 02-02-16 15:44:24, Mike Galbraith wrote:
> 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?

AFAICS [1] nothing...

---
[1] http://lkml.kernel.org/r/20160203122855.GB6762@dhcp22.suse.cz
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1325628

On Tue, Feb 02, 2016 at 03:44:24PM +0100, Mike Galbraith wrote:
> 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?

(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/1453702100-2597-1-git-send-email-tangchen@cn.fujitsu.com

I'll create a bandaid patch for now so that wq falls back to dfl_wq on
negative node.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

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.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Cc: Tang Chen <tangchen@cn.fujitsu.com>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Len Brown <len.brown@intel.com>
Cc: stable@vger.kernel.org # v4.3+
Fixes: 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
Link: http://lkml.kernel.org/g/1454424264.11183.46.camel@gmail.com
Link: http://lkml.kernel.org/g/1453702100-2597-1-git-send-email-tangchen@cn.fujitsu.com
---
 kernel/workqueue.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 61a0264..f748eab 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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]);
 }
 

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 03, 2016 at 01:54:25PM -0500, Tejun Heo wrote:
> 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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

On Wed, 2016-02-03 at 13:55 -0500, Tejun Heo wrote:
> On Wed, Feb 03, 2016 at 01:54:25PM -0500, Tejun Heo wrote:
> > 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

[toc] | [prev] | [next] | [standalone]

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

On Wed, 3 Feb 2016, Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Cc: Tang Chen <tangchen@cn.fujitsu.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: stable@vger.kernel.org # v4.3+

4.3+ ? Hasn't 874bbfe600a6 been backported to older stable kernels?

Adding a 'Fixes: 874bbfe600a6 ...' tag is what you really want here.

> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 61a0264..f748eab 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -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?

> +	 */
> +	if (unlikely(node == NUMA_NO_NODE))
> +		return wq->dfl_pwq;
> +

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

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

Hello,

On Wed, Feb 03, 2016 at 08:12:19PM +0100, Thomas Gleixner wrote:
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> > Cc: Tang Chen <tangchen@cn.fujitsu.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: stable@vger.kernel.org # v4.3+
> 
> 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.

> > @@ -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?

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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

On Wed, 2016-02-03 at 14:28 -0500, Tejun Heo wrote:
> Hello,
> 
> On Wed, Feb 03, 2016 at 08:12:19PM +0100, Thomas Gleixner wrote:
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> > > Cc: Tang Chen <tangchen@cn.fujitsu.com>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > > Cc: Len Brown <len.brown@intel.com>
> > > Cc: stable@vger.kernel.org # v4.3+
> > 
> > 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.
> 
> > > @@ -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?
> 
> 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

[toc] | [prev] | [next] | [standalone]

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

On Wed 03-02-16 13:54:25, Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Cc: Tang Chen <tangchen@cn.fujitsu.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: stable@vger.kernel.org # v4.3+
> Fixes: 874bbfe600a6 ("workqueue: make sure delayed work run in local cpu")
> Link: http://lkml.kernel.org/g/1454424264.11183.46.camel@gmail.com
> Link: http://lkml.kernel.org/g/1453702100-2597-1-git-send-email-tangchen@cn.fujitsu.com

Reviewed-by: Michal Hocko <mhocko@suse.com>

> ---
>  kernel/workqueue.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 61a0264..f748eab 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -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]);
>  }
>  

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web