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

Groups > linux.kernel > #1501046 > unrolled thread

Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-14 17:10 +0200
    Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Balbir Singh <bsingharora@gmail.com> - 2016-10-15 03:30 +0200
    Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Michael Ellerman <mpe@ellerman.id.au> - 2016-10-15 11:50 +0200
      Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-17 20:20 +0200
    Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Michael Ellerman <mpe@ellerman.id.au> - 2016-10-17 14:30 +0200
      Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Balbir Singh <bsingharora@gmail.com> - 2016-10-17 15:00 +0200
        Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Michael Ellerman <mpe@ellerman.id.au> - 2016-10-18 04:40 +0200
      Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-17 20:20 +0200
        Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-17 21:40 +0200
          Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue  available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-18 21:10 +0200
            Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Michael Ellerman <mpe@ellerman.id.au> - 2016-10-19 17:00 +0200
              [PATCH wq/for-4.10] workqueue: move wq_numa_init() to  workqueue_init() Tejun Heo <tj@kernel.org> - 2016-10-19 18:20 +0200

#1501046 — Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Hello, Michael.

On Tue, Oct 11, 2016 at 10:22:13PM +1100, Michael Ellerman wrote:
> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
> is NULL.
> 
> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
> set_task_rq() and happen to get NULL.
> 
> We never should have done set_task_rq(p, 2048), because 2048 is >=
> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
> doesn't cope with that.

Hmm... it doesn't reproduce it here and can't see how the commit would
affect this given that it doesn't really change when the kworker
kthreads are being created.

> Presumably we shouldn't be ending up with tsk_cpus_allowed() being
> empty, but I haven't had time to track down why that's happening.

Can you please add WARN_ON_ONCE(!tsk_nr_cpus_allowed(p)) to
select_task_rq() and post what that says?

Thanks.

-- 
tejun

[toc] | [next] | [standalone]

#1501241

On 15/10/16 02:07, Tejun Heo wrote:
> Hello, Michael.
> 
> On Tue, Oct 11, 2016 at 10:22:13PM +1100, Michael Ellerman wrote:
>> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
>> is NULL.
>>
>> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
>> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
>> set_task_rq() and happen to get NULL.
>>
>> We never should have done set_task_rq(p, 2048), because 2048 is >=
>> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
>> doesn't cope with that.
> 
> Hmm... it doesn't reproduce it here and can't see how the commit would
> affect this given that it doesn't really change when the kworker
> kthreads are being created.
> 
>> Presumably we shouldn't be ending up with tsk_cpus_allowed() being
>> empty, but I haven't had time to track down why that's happening.
> 

I think the basic analysis shows the change to creation of unbounded
workqueues from the unbound_hash, but those have a pool cpumask empty.

> Can you please add WARN_ON_ONCE(!tsk_nr_cpus_allowed(p)) to
> select_task_rq() and post what that says?
> 
> Thanks.
> 

Balbir Singh.

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

#1501264 — Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Tejun Heo <tj@kernel.org> writes:

> Hello, Michael.
>
> On Tue, Oct 11, 2016 at 10:22:13PM +1100, Michael Ellerman wrote:
>> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
>> is NULL.
>> 
>> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
>> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
>> set_task_rq() and happen to get NULL.
>> 
>> We never should have done set_task_rq(p, 2048), because 2048 is >=
>> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
>> doesn't cope with that.
>
> Hmm... it doesn't reproduce it here and can't see how the commit would
> affect this given that it doesn't really change when the kworker
> kthreads are being created.

Try turning on CONFIG_DEBUG_PER_CPU_MAPS=y ?

That will warn if you're indexing off the end of a cpu mask and just
getting lucky with the result.

>> Presumably we shouldn't be ending up with tsk_cpus_allowed() being
>> empty, but I haven't had time to track down why that's happening.
>
> Can you please add WARN_ON_ONCE(!tsk_nr_cpus_allowed(p)) to
> select_task_rq() and post what that says?

It says:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at ../kernel/sched/core.c:1602 try_to_wake_up+0x3f4/0x5c0
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-compiler_gcc-6.2.0-next-20161014-dirty #116
task: c000000ff9200000 task.stack: c000001ffc084000
NIP: c0000000000f1ba4 LR: c0000000000f180c CTR: 0000000000000000
REGS: c000001ffc0878f0 TRAP: 0700   Not tainted  (4.8.0-compiler_gcc-6.2.0-next-20161014-dirty)
MSR: 9000000002029033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE> CR: 28000422  XER: 00000000
CFAR: c0000000000f18bc SOFTE: 0 
GPR00: c0000000000f180c c000001ffc087b70 c000000000e83400 0000000000000000 
GPR04: 0000000000000002 0000000000000000 0000000000000000 0000000000000000 
GPR08: c000000000dc3400 0000000000000001 0000000000000002 0000000000000000 
GPR12: 0000000000000000 c00000000fb80000 c00000000000e0c8 0000000000000000 
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000000eb8960 
GPR24: 0000000000000000 c000000000d8ce00 0000000000000000 0000000000000000 
GPR28: c0000007f54050f4 0000000000000000 0000000000000000 c0000007f5404900 
NIP [c0000000000f1ba4] try_to_wake_up+0x3f4/0x5c0
LR [c0000000000f180c] try_to_wake_up+0x5c/0x5c0
Call Trace:
[c000001ffc087b70] [c0000000000f180c] try_to_wake_up+0x5c/0x5c0 (unreliable)
[c000001ffc087bf0] [c0000000000d53e4] create_worker+0x144/0x250
[c000001ffc087c90] [c000000000cf7930] workqueue_init+0x170/0x19c
[c000001ffc087d00] [c000000000ce0e74] kernel_init_freeable+0x158/0x360
[c000001ffc087dc0] [c00000000000e0e4] kernel_init+0x24/0x160
[c000001ffc087e30] [c00000000000bfa0] ret_from_kernel_thread+0x5c/0xbc
Instruction dump:
e8790890 4bff6ed9 2fa30000 419e00dc 60000000 4bfffe54 3d02fff4 8928d7f9 
2f890000 409e0018 39200001 9928d7f9 <0fe00000> 60000000 60420000 3b5f0368 
---[ end trace 0000000000000000 ]---


But I'm not sure that tells us anything new?

cheers

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

#1502328

Hello,

On Sat, Oct 15, 2016 at 08:48:01PM +1100, Michael Ellerman wrote:
> > Hmm... it doesn't reproduce it here and can't see how the commit would
> > affect this given that it doesn't really change when the kworker
> > kthreads are being created.
> 
> Try turning on CONFIG_DEBUG_PER_CPU_MAPS=y ?
> 
> That will warn if you're indexing off the end of a cpu mask and just
> getting lucky with the result.

That's not happening on x86.  That could mean that powerpc is
initializing cpu_possible_mask after workqueue_init_early().  Looking
into it.

> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at ../kernel/sched/core.c:1602 try_to_wake_up+0x3f4/0x5c0
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-compiler_gcc-6.2.0-next-20161014-dirty #116
> task: c000000ff9200000 task.stack: c000001ffc084000
> NIP: c0000000000f1ba4 LR: c0000000000f180c CTR: 0000000000000000
> REGS: c000001ffc0878f0 TRAP: 0700   Not tainted  (4.8.0-compiler_gcc-6.2.0-next-20161014-dirty)
> MSR: 9000000002029033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE> CR: 28000422  XER: 00000000
> CFAR: c0000000000f18bc SOFTE: 0 
> GPR00: c0000000000f180c c000001ffc087b70 c000000000e83400 0000000000000000 
> GPR04: 0000000000000002 0000000000000000 0000000000000000 0000000000000000 
> GPR08: c000000000dc3400 0000000000000001 0000000000000002 0000000000000000 
> GPR12: 0000000000000000 c00000000fb80000 c00000000000e0c8 0000000000000000 
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
> GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000000eb8960 
> GPR24: 0000000000000000 c000000000d8ce00 0000000000000000 0000000000000000 
> GPR28: c0000007f54050f4 0000000000000000 0000000000000000 c0000007f5404900 
> NIP [c0000000000f1ba4] try_to_wake_up+0x3f4/0x5c0
> LR [c0000000000f180c] try_to_wake_up+0x5c/0x5c0
> Call Trace:
> [c000001ffc087b70] [c0000000000f180c] try_to_wake_up+0x5c/0x5c0 (unreliable)
> [c000001ffc087bf0] [c0000000000d53e4] create_worker+0x144/0x250
> [c000001ffc087c90] [c000000000cf7930] workqueue_init+0x170/0x19c
> [c000001ffc087d00] [c000000000ce0e74] kernel_init_freeable+0x158/0x360
> [c000001ffc087dc0] [c00000000000e0e4] kernel_init+0x24/0x160
> [c000001ffc087e30] [c00000000000bfa0] ret_from_kernel_thread+0x5c/0xbc
> Instruction dump:
> e8790890 4bff6ed9 2fa30000 419e00dc 60000000 4bfffe54 3d02fff4 8928d7f9 
> 2f890000 409e0018 39200001 9928d7f9 <0fe00000> 60000000 60420000 3b5f0368 
> ---[ end trace 0000000000000000 ]---
> 
> But I'm not sure that tells us anything new?

Yeah, I should have asked to print out information of the target task
but it looks like we have enough information now.

Thanks.

-- 
tejun

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

#1501930 — Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Tejun Heo <tj@kernel.org> writes:

> Hello, Michael.
>
> On Tue, Oct 11, 2016 at 10:22:13PM +1100, Michael Ellerman wrote:
>> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
>> is NULL.
>> 
>> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
>> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
>> set_task_rq() and happen to get NULL.
>> 
>> We never should have done set_task_rq(p, 2048), because 2048 is >=
>> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
>> doesn't cope with that.
>
> Hmm... it doesn't reproduce it here and can't see how the commit would
> affect this given that it doesn't really change when the kworker
> kthreads are being created.

It changes when the pool attributes are created, which is the source of
the bug.

The original crash happens because we have a task with an empty cpus_allowed
mask. That mask originally comes from pool->attrs->cpumask.

The attrs for the pool are created early via workqueue_init_early() in
apply_wqattrs_prepare():

  start_here_common
  -> start_kernel
     -> workqueue_init_early
        -> __alloc_workqueue_key
           -> apply_workqueue_attrs
              -> apply_workqueue_attrs_locked
                 -> apply_wqattrs_prepare
	          
In there we do:

	copy_workqueue_attrs(new_attrs, attrs);
	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, wq_unbound_cpumask);
	if (unlikely(cpumask_empty(new_attrs->cpumask)))
		cpumask_copy(new_attrs->cpumask, wq_unbound_cpumask);
	...
	copy_workqueue_attrs(tmp_attrs, new_attrs);
	...
	for_each_node(node) {
		if (wq_calc_node_cpumask(new_attrs, node, -1, tmp_attrs->cpumask)) {
+			BUG_ON(cpumask_empty(tmp_attrs->cpumask));
			ctx->pwq_tbl[node] = alloc_unbound_pwq(wq, tmp_attrs);


The bad case (where we hit the BUG_ON I added above) is where we are
creating a wq for node 1.

In wq_calc_node_cpumask() we do:

	cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
	return !cpumask_equal(cpumask, attrs->cpumask);

Which with the arguments inserted is:

	cpumask_and(tmp_attrs->cpumask, new_attrs->cpumask, wq_numa_possible_cpumask[1]);
	return !cpumask_equal(tmp_attrs->cpumask, new_attrs->cpumask);

And that results in tmp_attrs->cpumask being empty, because
wq_numa_possible_cpumask[1] is an empty cpumask.

The reason wq_numa_possible_cpumask[1] is an empty mask is because in
wq_numa_init() we did:

	for_each_possible_cpu(cpu) {
		node = cpu_to_node(cpu);
		if (WARN_ON(node == NUMA_NO_NODE)) {
			pr_warn("workqueue: NUMA node mapping not available for cpu%d, disabling NUMA support\n", cpu);
			/* happens iff arch is bonkers, let's just proceed */
			return;
		}
		cpumask_set_cpu(cpu, tbl[node]);
	}

And cpu_to_node() returned node 0 for every CPU in the system, despite there
being multiple nodes.

That happened because we haven't yet called set_cpu_numa_node() for the non-boot
cpus, because that happens in smp_prepare_cpus(), and
workqueue_init_early() is called much earlier than that.

This doesn't trigger on x86 because it does set_cpu_numa_node() in
setup_per_cpu_areas(), which is called prior to workqueue_init_early().

We can (should) probably do the same on powerpc, I'll look at that
tomorrow. But other arches may have a similar problem, and at the very
least we need to document that workqueue_init_early() relies on
cpu_to_node() working.

cheers

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

#1501954

On 17/10/16 23:24, Michael Ellerman wrote:
> Tejun Heo <tj@kernel.org> writes:
> 
>> Hello, Michael.
>>
>> On Tue, Oct 11, 2016 at 10:22:13PM +1100, Michael Ellerman wrote:
>>> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
>>> is NULL.
>>>
>>> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
>>> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
>>> set_task_rq() and happen to get NULL.
>>>
>>> We never should have done set_task_rq(p, 2048), because 2048 is >=
>>> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
>>> doesn't cope with that.
>>
>> Hmm... it doesn't reproduce it here and can't see how the commit would
>> affect this given that it doesn't really change when the kworker
>> kthreads are being created.
> 
> It changes when the pool attributes are created, which is the source of
> the bug.
> 
> The original crash happens because we have a task with an empty cpus_allowed
> mask. That mask originally comes from pool->attrs->cpumask.
> 
> The attrs for the pool are created early via workqueue_init_early() in
> apply_wqattrs_prepare():
> 
>   start_here_common
>   -> start_kernel
>      -> workqueue_init_early
>         -> __alloc_workqueue_key
>            -> apply_workqueue_attrs
>               -> apply_workqueue_attrs_locked
>                  -> apply_wqattrs_prepare
> 	          
> In there we do:
> 
> 	copy_workqueue_attrs(new_attrs, attrs);
> 	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, wq_unbound_cpumask);
> 	if (unlikely(cpumask_empty(new_attrs->cpumask)))
> 		cpumask_copy(new_attrs->cpumask, wq_unbound_cpumask);
> 	...
> 	copy_workqueue_attrs(tmp_attrs, new_attrs);
> 	...
> 	for_each_node(node) {
> 		if (wq_calc_node_cpumask(new_attrs, node, -1, tmp_attrs->cpumask)) {
> +			BUG_ON(cpumask_empty(tmp_attrs->cpumask));
> 			ctx->pwq_tbl[node] = alloc_unbound_pwq(wq, tmp_attrs);
> 
> 
> The bad case (where we hit the BUG_ON I added above) is where we are
> creating a wq for node 1.
> 
> In wq_calc_node_cpumask() we do:
> 
> 	cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
> 	return !cpumask_equal(cpumask, attrs->cpumask);
> 
> Which with the arguments inserted is:
> 
> 	cpumask_and(tmp_attrs->cpumask, new_attrs->cpumask, wq_numa_possible_cpumask[1]);
> 	return !cpumask_equal(tmp_attrs->cpumask, new_attrs->cpumask);
> 
> And that results in tmp_attrs->cpumask being empty, because
> wq_numa_possible_cpumask[1] is an empty cpumask.
> 
> The reason wq_numa_possible_cpumask[1] is an empty mask is because in
> wq_numa_init() we did:
> 
> 	for_each_possible_cpu(cpu) {
> 		node = cpu_to_node(cpu);
> 		if (WARN_ON(node == NUMA_NO_NODE)) {
> 			pr_warn("workqueue: NUMA node mapping not available for cpu%d, disabling NUMA support\n", cpu);
> 			/* happens iff arch is bonkers, let's just proceed */
> 			return;
> 		}
> 		cpumask_set_cpu(cpu, tbl[node]);
> 	}
> 
> And cpu_to_node() returned node 0 for every CPU in the system, despite there
> being multiple nodes.
> 
> That happened because we haven't yet called set_cpu_numa_node() for the non-boot
> cpus, because that happens in smp_prepare_cpus(), and
> workqueue_init_early() is called much earlier than that.
> 
> This doesn't trigger on x86 because it does set_cpu_numa_node() in
> setup_per_cpu_areas(), which is called prior to workqueue_init_early().
> 
> We can (should) probably do the same on powerpc, I'll look at that
> tomorrow. But other arches may have a similar problem, and at the very
> least we need to document that workqueue_init_early() relies on
> cpu_to_node() working.

Don't we do the setup cpu->node mapings in initmem_init()?
Ideally we have setup_arch->intmem_init->numa_setup_cpu

Will look at it tomorrow
Balbir Singh

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

#1502663 — Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Balbir Singh <bsingharora@gmail.com> writes:
> On 17/10/16 23:24, Michael Ellerman wrote:
>> That happened because we haven't yet called set_cpu_numa_node() for the non-boot
>> cpus, because that happens in smp_prepare_cpus(), and
>> workqueue_init_early() is called much earlier than that.
>> 
>> This doesn't trigger on x86 because it does set_cpu_numa_node() in
>> setup_per_cpu_areas(), which is called prior to workqueue_init_early().
>> 
>> We can (should) probably do the same on powerpc, I'll look at that
>> tomorrow. But other arches may have a similar problem, and at the very
>> least we need to document that workqueue_init_early() relies on
>> cpu_to_node() working.
>
> Don't we do the setup cpu->node mapings in initmem_init()?
> Ideally we have setup_arch->intmem_init->numa_setup_cpu

That sets up numa_cpu_lookup_table, which is a powerpc only data
structure.

But it doesn't setup the percpu numa_node variables, used by
cpu_to_node(), because percpu areas are not setup yet.

cheers

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

#1502327

Hello, Michael.

On Mon, Oct 17, 2016 at 11:24:34PM +1100, Michael Ellerman wrote:
> The bad case (where we hit the BUG_ON I added above) is where we are
> creating a wq for node 1.
> 
> In wq_calc_node_cpumask() we do:
> 
> 	cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
> 	return !cpumask_equal(cpumask, attrs->cpumask);
> 
> Which with the arguments inserted is:
> 
> 	cpumask_and(tmp_attrs->cpumask, new_attrs->cpumask, wq_numa_possible_cpumask[1]);
> 	return !cpumask_equal(tmp_attrs->cpumask, new_attrs->cpumask);
> 
> And that results in tmp_attrs->cpumask being empty, because
> wq_numa_possible_cpumask[1] is an empty cpumask.

Ah, should have read this before replying to the previous mail, so
it's the numa mask, not the cpu_possible_mask.

> The reason wq_numa_possible_cpumask[1] is an empty mask is because in
> wq_numa_init() we did:
> 
> 	for_each_possible_cpu(cpu) {
> 		node = cpu_to_node(cpu);
> 		if (WARN_ON(node == NUMA_NO_NODE)) {
> 			pr_warn("workqueue: NUMA node mapping not available for cpu%d, disabling NUMA support\n", cpu);
> 			/* happens iff arch is bonkers, let's just proceed */
> 			return;
> 		}
> 		cpumask_set_cpu(cpu, tbl[node]);
> 	}
> 
> And cpu_to_node() returned node 0 for every CPU in the system, despite there
> being multiple nodes.
> 
> That happened because we haven't yet called set_cpu_numa_node() for the non-boot
> cpus, because that happens in smp_prepare_cpus(), and
> workqueue_init_early() is called much earlier than that.
> 
> This doesn't trigger on x86 because it does set_cpu_numa_node() in
> setup_per_cpu_areas(), which is called prior to workqueue_init_early().
> 
> We can (should) probably do the same on powerpc, I'll look at that
> tomorrow. But other arches may have a similar problem, and at the very
> least we need to document that workqueue_init_early() relies on
> cpu_to_node() working.

I should be able to move the numa part of initialization to the later
init function.  Working on it.

Thanks.

-- 
tejun

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

#1502376

Hello, Michael.

Other NUMA archs are lazy-initializing cpu to node mapping too, so we
need to fix it from workqueue side.  This also means that we've been
getting NUMA node wrong for percpu pools on those archs.

Can you please try the following patch and if it resolves the issue,
report the workqueue part (it's at the end) of sysrq-t dump?

Thanks.

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 984f6ff..276557b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4411,14 +4411,14 @@ void show_workqueue_state(void)
 				break;
 			}
 		}
-		if (idle)
-			continue;
+		//if (idle)
+		//	continue;
 
 		pr_info("workqueue %s: flags=0x%x\n", wq->name, wq->flags);
 
 		for_each_pwq(pwq, wq) {
 			spin_lock_irqsave(&pwq->pool->lock, flags);
-			if (pwq->nr_active || !list_empty(&pwq->delayed_works))
+			//if (pwq->nr_active || !list_empty(&pwq->delayed_works))
 				show_pwq(pwq);
 			spin_unlock_irqrestore(&pwq->pool->lock, flags);
 		}
@@ -4429,8 +4429,8 @@ void show_workqueue_state(void)
 		bool first = true;
 
 		spin_lock_irqsave(&pool->lock, flags);
-		if (pool->nr_workers == pool->nr_idle)
-			goto next_pool;
+		//if (pool->nr_workers == pool->nr_idle)
+		//	goto next_pool;
 
 		pr_info("pool %d:", pool->id);
 		pr_cont_pool_info(pool);
@@ -4649,10 +4649,12 @@ int workqueue_online_cpu(unsigned int cpu)
 	for_each_pool(pool, pi) {
 		mutex_lock(&pool->attach_mutex);
 
-		if (pool->cpu == cpu)
+		if (pool->cpu == cpu) {
+			pool->node = cpu_to_node(cpu);
 			rebind_workers(pool);
-		else if (pool->cpu < 0)
+		} else if (pool->cpu < 0) {
 			restore_unbound_workers_cpumask(pool, cpu);
+		}
 
 		mutex_unlock(&pool->attach_mutex);
 	}
@@ -5495,8 +5497,6 @@ int __init workqueue_init_early(void)
 
 	pwq_cache = KMEM_CACHE(pool_workqueue, SLAB_PANIC);
 
-	wq_numa_init();
-
 	/* initialize CPU pools */
 	for_each_possible_cpu(cpu) {
 		struct worker_pool *pool;
@@ -5571,6 +5571,9 @@ int __init workqueue_init(void)
 	struct worker_pool *pool;
 	int cpu, bkt;
 
+	wq_numa_init();
+	wq_update_unbound_numa(wq, smp_processor_id(), true);
+
 	/* create the initial workers */
 	for_each_online_cpu(cpu) {
 		for_each_cpu_worker_pool(pool, cpu) {

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

#1503297

Hello, Michael.

On Tue, Oct 18, 2016 at 03:37:42PM +1100, Michael Ellerman wrote:
> That doesn't compile, wq doesn't exist.
> 
> I guessed that you meant:
> 
> +       wq_numa_init();
> +       list_for_each_entry(wq, &workqueues, list)
> +               wq_update_unbound_numa(wq, smp_processor_id(), true);

Yeap, sorry about that.

> And that does boot.
> 
> The sysrq-t output is below, it's rather large.

Didn't expect that many cpus but it looks good.  I'll post proper
patches soon.

Thanks!

-- 
tejun

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

#1503805 — Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot)

Tejun Heo <tj@kernel.org> writes:

> Hello, Michael.
>
> On Tue, Oct 18, 2016 at 03:37:42PM +1100, Michael Ellerman wrote:
>> That doesn't compile, wq doesn't exist.
>> 
>> I guessed that you meant:
>> 
>> +       wq_numa_init();
>> +       list_for_each_entry(wq, &workqueues, list)
>> +               wq_update_unbound_numa(wq, smp_processor_id(), true);
>
> Yeap, sorry about that.

No worries.

>> And that does boot.
>> 
>> The sysrq-t output is below, it's rather large.
>
> Didn't expect that many cpus but it looks good.

I have another system here with twice as many CPUs if that would help ;)

> I'll post proper patches soon.

Thanks. Can you pull the current series out of linux-next for now until
you merge the new series?

cheers

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

#1504060 — [PATCH wq/for-4.10] workqueue: move wq_numa_init() to workqueue_init()

Hello, Michael.

I simplified the patch a bit and applied the following to for-4.10 and
updated for-next accordingly.  The coming next snapshot should be
fine.

Thanks!

------ 8< ------
From 2186d9f940b6a04f263a3bacd48f2a7ba96df4cf Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Wed, 19 Oct 2016 12:01:27 -0400

While splitting up workqueue initialization into two parts,
ac8f73400782 ("workqueue: make workqueue available early during boot")
put wq_numa_init() into workqueue_init_early().  Unfortunately, on
some archs including power and arm64, cpu to node mapping isn't yet
established by the time the early init is called leading to incorrect
NUMA initialization and subsequently the following oops due to zero
cpumask on node-specific unbound pools.

  Unable to handle kernel paging request for data at address 0x00000038
  Faulting instruction address: 0xc0000000000fc0cc
  Oops: Kernel access of bad area, sig: 11 [#1]
  SMP NR_CPUS=2048 NUMA PowerNV
  Modules linked in:
  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-compiler_gcc-6.2.0-next-20161005 #94
  task: c0000007f5400000 task.stack: c000001ffc084000
  NIP: c0000000000fc0cc LR: c0000000000ed928 CTR: c0000000000fbfd0
  REGS: c000001ffc087780 TRAP: 0300   Not tainted  (4.8.0-compiler_gcc-6.2.0-next-20161005)
  MSR: 9000000002009033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE>  CR: 48000424  XER: 00000000
  CFAR: c0000000000089dc DAR: 0000000000000038 DSISR: 40000000 SOFTE: 0
  GPR00: c0000000000ed928 c000001ffc087a00 c000000000e63200 c000000010d6d600
  GPR04: c0000007f5409200 0000000000000021 000000000748e08c 000000000000001f
  GPR08: 0000000000000000 0000000000000021 000000000748f1f8 0000000000000000
  GPR12: 0000000028000422 c00000000fb80000 c00000000000e0c8 0000000000000000
  GPR16: 0000000000000000 0000000000000000 0000000000000021 0000000000000001
  GPR20: ffffffffafb50401 0000000000000000 c000000010d6d600 000000000000ba7e
  GPR24: 000000000000ba7e c000000000d8bc58 afb504000afb5041 0000000000000001
  GPR28: 0000000000000000 0000000000000004 c0000007f5409280 0000000000000000
  NIP [c0000000000fc0cc] enqueue_task_fair+0xfc/0x18b0
  LR [c0000000000ed928] activate_task+0x78/0xe0
  Call Trace:
  [c000001ffc087a00] [c0000007f5409200] 0xc0000007f5409200 (unreliable)
  [c000001ffc087b10] [c0000000000ed928] activate_task+0x78/0xe0
  [c000001ffc087b50] [c0000000000ede58] ttwu_do_activate+0x68/0xc0
  [c000001ffc087b90] [c0000000000ef1b8] try_to_wake_up+0x208/0x4f0
  [c000001ffc087c10] [c0000000000d3484] create_worker+0x144/0x250
  [c000001ffc087cb0] [c000000000cd72d0] workqueue_init+0x124/0x150
  [c000001ffc087d00] [c000000000cc0e74] kernel_init_freeable+0x158/0x360
  [c000001ffc087dc0] [c00000000000e0e4] kernel_init+0x24/0x160
  [c000001ffc087e30] [c00000000000bfa0] ret_from_kernel_thread+0x5c/0xbc
  Instruction dump:
  62940401 3b800000 3aa00000 7f17c378 3a600001 3b600001 60000000 60000000
  60420000 72490021 ebfe0150 2f890001 <ebbf0038> 419e0de0 7fbee840 419e0e58
  ---[ end trace 0000000000000000 ]---

Fix it by moving wq_numa_init() to workqueue_init().  As this means
that the early intialization may not have full NUMA info for per-cpu
pools and ignores NUMA affinity for unbound pools, fix them up from
workqueue_init() after wq_numa_init().

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Link: http://lkml.kernel.org/r/87twck5wqo.fsf@concordia.ellerman.id.au
Fixes: ac8f73400782 ("workqueue: make workqueue available early during boot")
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ad0cd43..c56479b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5495,8 +5495,6 @@ int __init workqueue_init_early(void)
 
 	pwq_cache = KMEM_CACHE(pool_workqueue, SLAB_PANIC);
 
-	wq_numa_init();
-
 	/* initialize CPU pools */
 	for_each_possible_cpu(cpu) {
 		struct worker_pool *pool;
@@ -5566,9 +5564,32 @@ int __init workqueue_init_early(void)
  */
 int __init workqueue_init(void)
 {
+	struct workqueue_struct *wq;
 	struct worker_pool *pool;
 	int cpu, bkt;
 
+	/*
+	 * It'd be simpler to initialize NUMA in workqueue_init_early() but
+	 * CPU to node mapping may not be available that early on some
+	 * archs such as power and arm64.  As per-cpu pools created
+	 * previously could be missing node hint and unbound pools NUMA
+	 * affinity, fix them up.
+	 */
+	wq_numa_init();
+
+	mutex_lock(&wq_pool_mutex);
+
+	for_each_possible_cpu(cpu) {
+		for_each_cpu_worker_pool(pool, cpu) {
+			pool->node = cpu_to_node(cpu);
+		}
+	}
+
+	list_for_each_entry(wq, &workqueues, list)
+		wq_update_unbound_numa(wq, smp_processor_id(), true);
+
+	mutex_unlock(&wq_pool_mutex);
+
 	/* create the initial workers */
 	for_each_online_cpu(cpu) {
 		for_each_cpu_worker_pool(pool, cpu) {
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web