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

Groups > linux.kernel > #1641830 > unrolled thread

Re: [PATCH] workqueue: Ensure that cpumask set for pools created after 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: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Michael Bringmann <mwb@linux.vnet.ibm.com> - 2017-05-15 17:50 +0200
    Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Tejun Heo <tj@kernel.org> - 2017-05-16 18:00 +0200
      Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Michael Bringmann <mwb@linux.vnet.ibm.com> - 2017-05-23 21:50 +0200
        Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Tejun Heo <tj@kernel.org> - 2017-05-23 22:00 +0200
          Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Michael Bringmann <mwb@linux.vnet.ibm.com> - 2017-05-23 22:10 +0200
            Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Tejun Heo <tj@kernel.org> - 2017-05-23 22:20 +0200
              Re: [PATCH] workqueue: Ensure that cpumask set for pools created  after boot Michael Bringmann <mwb@linux.vnet.ibm.com> - 2017-05-24 18:40 +0200

#1641830 — Re: [PATCH] workqueue: Ensure that cpumask set for pools created after boot

Hello:

On 05/10/2017 12:33 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, May 10, 2017 at 11:48:17AM -0500, Michael Bringmann wrote:
>>
>> On NUMA systems with dynamic processors, the content of the cpumask
>> may change over time.  As new processors are added via DLPAR operations,
>> workqueues are created for them.  This patch ensures that the pools
>> created for new workqueues will be initialized with a cpumask before
>> the first worker is created, attached, and woken up.  If the mask is
>> not set up, then the kernel will crash when 'wakeup_process' is unable
>> to find a valid CPU to which to assign the new worker.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index c74bf39..6091069 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -3366,6 +3366,8 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
>>  	copy_workqueue_attrs(pool->attrs, attrs);
>>  	pool->node = target_node;
>>  
>> +	cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
> 
> What prevents a cpu getting added right here tho?

PowerPC has only one control path to add/remove CPUs via DLPAR operations.
Even so, the underlying code is protected through multiple locks.

> 
> Maybe the right thing to do is protecting the whole thing with hotplug
> readlock?

The operation is already within a hotplug readlock when performing DLPAR
add/remove.  Adding a CPU to the system, requires it to be brought online.
Removing a CPU from the system, requires it to be taken offline.  These
involve calls to cpu_up / cpu_down, which go through _cpu_up / _cpu_down,
which acquire the hotplug locks, among others along the path of execution.

The locks are acquired before getting to the workqueue code, the pool
creation/attachment code (which is where the cpu mask needs to be set),
or trying to wakeup the initial created task in 'sched.c'.

> 
> Thanks.
> 

Regards,
Michael

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

[toc] | [next] | [standalone]

#1642627

Hello, Michael.

On Mon, May 15, 2017 at 10:48:04AM -0500, Michael Bringmann wrote:
> >> --- a/kernel/workqueue.c
> >> +++ b/kernel/workqueue.c
> >> @@ -3366,6 +3366,8 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
> >>  	copy_workqueue_attrs(pool->attrs, attrs);
> >>  	pool->node = target_node;
> >>  
> >> +	cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
> > 
> > What prevents a cpu getting added right here tho?
> 
> PowerPC has only one control path to add/remove CPUs via DLPAR operations.
> Even so, the underlying code is protected through multiple locks.

The more I look at the patch, the less sense it seems to make.  So,
whenever we create a new pool, we ignore the requested cpumask and
override it with the cpumask of the current thread?

> > Maybe the right thing to do is protecting the whole thing with hotplug
> > readlock?
> 
> The operation is already within a hotplug readlock when performing DLPAR
> add/remove.  Adding a CPU to the system, requires it to be brought online.
> Removing a CPU from the system, requires it to be taken offline.  These
> involve calls to cpu_up / cpu_down, which go through _cpu_up / _cpu_down,
> which acquire the hotplug locks, among others along the path of execution.
> 
> The locks are acquired before getting to the workqueue code, the pool
> creation/attachment code (which is where the cpu mask needs to be set),
> or trying to wakeup the initial created task in 'sched.c'.

A new unbound workqueue and thus unbound pool can also be created from
paths outside cpu hotplug, so get_unbound_pool() can race against
hotplug.  Can you please explain the failures that you see in more
detail?  I'm sure your patch works around the issue somehow but it
doesn't look like the right fix.

Thanks.

-- 
tejun

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

#1648372

On 05/16/2017 10:55 AM, Tejun Heo wrote:
> Hello, Michael.
> 
> On Mon, May 15, 2017 at 10:48:04AM -0500, Michael Bringmann wrote:
>>>> --- a/kernel/workqueue.c
>>>> +++ b/kernel/workqueue.c
>>>> @@ -3366,6 +3366,8 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
>>>>  	copy_workqueue_attrs(pool->attrs, attrs);
>>>>  	pool->node = target_node;
>>>>  
>>>> +	cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
>>>
>>> What prevents a cpu getting added right here tho?
>>
>> PowerPC has only one control path to add/remove CPUs via DLPAR operations.
>> Even so, the underlying code is protected through multiple locks.
> 
> The more I look at the patch, the less sense it seems to make.  So,
> whenever we create a new pool, we ignore the requested cpumask and
> override it with the cpumask of the current thread?

No.  As I mentioned previously, the operation/problem occurs within a DLPAR
hotplug add/remove operation.  This is happening to a node which previously
did not have any CPUs associated to it -- we are trying to add more resources
to an LPAR / partition.  At this point, the cpumask for the node is empty / zero.
Sorry for not being more clear on this point earlier.
 
>>> Maybe the right thing to do is protecting the whole thing with hotplug
>>> readlock?
>>
>> The operation is already within a hotplug readlock when performing DLPAR
>> add/remove.  Adding a CPU to the system, requires it to be brought online.
>> Removing a CPU from the system, requires it to be taken offline.  These
>> involve calls to cpu_up / cpu_down, which go through _cpu_up / _cpu_down,
>> which acquire the hotplug locks, among others along the path of execution.
>>
>> The locks are acquired before getting to the workqueue code, the pool
>> creation/attachment code (which is where the cpu mask needs to be set),
>> or trying to wakeup the initial created task in 'sched.c'.
> 
> A new unbound workqueue and thus unbound pool can also be created from
> paths outside cpu hotplug, so get_unbound_pool() can race against
> hotplug.  Can you please explain the failures that you see in more
> detail?  I'm sure your patch works around the issue somehow but it
> doesn't look like the right fix.

We fill in an empty cpumask field with a guaranteed non-empty value.
I verified that the incoming cpumask in the attrs was zero at this point
preceding the failure.  If we proceed without putting in a useful value,
we go to 'wake_up_process()' (kernel/sched/core.c) next to wakeup the new
worker for the new unbound pool.  While there, the code runs through
'select_task_rq()' and invokes cpumask_any() on a copy of the cpumask.
Unfortunately, running that function over an empty/non-initialized cpumask
returns an index beyond the end of the list, resulting shortly thereafter
in an instruction/data fetch exception.

If you have a suggestion for an alternate non-empty value to use, I would
be happy to try it.

> 
> Thanks.
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

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

#1648380

Hello, Michael.

On Tue, May 23, 2017 at 02:44:23PM -0500, Michael Bringmann wrote:
> On 05/16/2017 10:55 AM, Tejun Heo wrote:
> > Hello, Michael.
> > 
> > On Mon, May 15, 2017 at 10:48:04AM -0500, Michael Bringmann wrote:
> >>>> --- a/kernel/workqueue.c
> >>>> +++ b/kernel/workqueue.c
> >>>> @@ -3366,6 +3366,8 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
> >>>>  	copy_workqueue_attrs(pool->attrs, attrs);
> >>>>  	pool->node = target_node;
> >>>>  
> >>>> +	cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
> >>>
> >>> What prevents a cpu getting added right here tho?
> >>
> >> PowerPC has only one control path to add/remove CPUs via DLPAR operations.
> >> Even so, the underlying code is protected through multiple locks.
> > 
> > The more I look at the patch, the less sense it seems to make.  So,
> > whenever we create a new pool, we ignore the requested cpumask and
> > override it with the cpumask of the current thread?
> 
> No.  As I mentioned previously, the operation/problem occurs within a DLPAR
> hotplug add/remove operation.  This is happening to a node which previously

But that's what the code is doing.  Whenever it creates a new unbound
pool, it ends up ignoring the requested cpumask and overwrites it with
the cpumask containing self.

> did not have any CPUs associated to it -- we are trying to add more resources
> to an LPAR / partition.  At this point, the cpumask for the node is empty / zero.
> Sorry for not being more clear on this point earlier.
...
> > A new unbound workqueue and thus unbound pool can also be created from
> > paths outside cpu hotplug, so get_unbound_pool() can race against
> > hotplug.  Can you please explain the failures that you see in more
> > detail?  I'm sure your patch works around the issue somehow but it
> > doesn't look like the right fix.
> 
> We fill in an empty cpumask field with a guaranteed non-empty value.
> I verified that the incoming cpumask in the attrs was zero at this point
> preceding the failure.  If we proceed without putting in a useful value,
> we go to 'wake_up_process()' (kernel/sched/core.c) next to wakeup the new
> worker for the new unbound pool.  While there, the code runs through
> 'select_task_rq()' and invokes cpumask_any() on a copy of the cpumask.
> Unfortunately, running that function over an empty/non-initialized cpumask
> returns an index beyond the end of the list, resulting shortly thereafter
> in an instruction/data fetch exception.
> 
> If you have a suggestion for an alternate non-empty value to use, I would
> be happy to try it.

Can you please post the backtrace of the problematic worker pool being
created (WARN_ON empty cpumask while creating a new pool)?

Thanks.

-- 
tejun

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

#1648382

To confirm, you want the WARN_ON(cpumask_any(pool->attrs->cpumask) >= NR_CPUS)
at the point where I place my current patch?

On 05/23/2017 02:49 PM, Tejun Heo wrote:
> Hello, Michael.
> 
> On Tue, May 23, 2017 at 02:44:23PM -0500, Michael Bringmann wrote:
>> On 05/16/2017 10:55 AM, Tejun Heo wrote:
>>> Hello, Michael.
>>>
>>> On Mon, May 15, 2017 at 10:48:04AM -0500, Michael Bringmann wrote:
>>>>>> --- a/kernel/workqueue.c
>>>>>> +++ b/kernel/workqueue.c
>>>>>> @@ -3366,6 +3366,8 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
>>>>>>  	copy_workqueue_attrs(pool->attrs, attrs);
>>>>>>  	pool->node = target_node;
>>>>>>  
>>>>>> +	cpumask_copy(pool->attrs->cpumask, cpumask_of(smp_processor_id()));
>>>>>
>>>>> What prevents a cpu getting added right here tho?
>>>>
>>>> PowerPC has only one control path to add/remove CPUs via DLPAR operations.
>>>> Even so, the underlying code is protected through multiple locks.
>>>
>>> The more I look at the patch, the less sense it seems to make.  So,
>>> whenever we create a new pool, we ignore the requested cpumask and
>>> override it with the cpumask of the current thread?
>>
>> No.  As I mentioned previously, the operation/problem occurs within a DLPAR
>> hotplug add/remove operation.  This is happening to a node which previously
> 
> But that's what the code is doing.  Whenever it creates a new unbound
> pool, it ends up ignoring the requested cpumask and overwrites it with
> the cpumask containing self.
> 
>> did not have any CPUs associated to it -- we are trying to add more resources
>> to an LPAR / partition.  At this point, the cpumask for the node is empty / zero.
>> Sorry for not being more clear on this point earlier.
> ...
>>> A new unbound workqueue and thus unbound pool can also be created from
>>> paths outside cpu hotplug, so get_unbound_pool() can race against
>>> hotplug.  Can you please explain the failures that you see in more
>>> detail?  I'm sure your patch works around the issue somehow but it
>>> doesn't look like the right fix.
>>
>> We fill in an empty cpumask field with a guaranteed non-empty value.
>> I verified that the incoming cpumask in the attrs was zero at this point
>> preceding the failure.  If we proceed without putting in a useful value,
>> we go to 'wake_up_process()' (kernel/sched/core.c) next to wakeup the new
>> worker for the new unbound pool.  While there, the code runs through
>> 'select_task_rq()' and invokes cpumask_any() on a copy of the cpumask.
>> Unfortunately, running that function over an empty/non-initialized cpumask
>> returns an index beyond the end of the list, resulting shortly thereafter
>> in an instruction/data fetch exception.
>>
>> If you have a suggestion for an alternate non-empty value to use, I would
>> be happy to try it.
> 
> Can you please post the backtrace of the problematic worker pool being
> created (WARN_ON empty cpumask while creating a new pool)?
> 
> Thanks.
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

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

#1648451

Hello,

On Tue, May 23, 2017 at 03:09:07PM -0500, Michael Bringmann wrote:
> To confirm, you want the WARN_ON(cpumask_any(pool->attrs->cpumask) >= NR_CPUS)
> at the point where I place my current patch?

Yeah, cpumask_weight() probably is a bit more intuitive but I'm
curious why we're creating workqueues for a node before cpus come
online.

Thanks.

-- 
tejun

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

#1649755

On 05/23/2017 03:10 PM, Tejun Heo wrote:
> Hello,
> 
> On Tue, May 23, 2017 at 03:09:07PM -0500, Michael Bringmann wrote:
>> To confirm, you want the WARN_ON(cpumask_any(pool->attrs->cpumask) >= NR_CPUS)
>> at the point where I place my current patch?
> 
> Yeah, cpumask_weight() probably is a bit more intuitive but I'm
> curious why we're creating workqueues for a node before cpus come
> online.
> 
> Thanks.
> 

I am in the middle of another test, but I did find this test crash log from
one of my earlier tests.  The system was configured for Shared Processors,
booting with 16 or so VPs, and then I was adding and removing them, and hit
this crash.  I will get the other log later.

[    8.599437] Unable to handle kernel paging request for unaligned access at address 0xc0000003c52231cf
[    8.599443] Faulting instruction address: 0xc00000000049c54c
[    8.599450] Oops: Kernel access of bad area, sig: 7 [#1]
[    8.599454] SMP NR_CPUS=2048 
[    8.599455] NUMA 
[    8.599458] pSeries
[    8.599463] Modules linked in:
[    8.599470] CPU: 35 PID: 1 Comm: swapper/0 Not tainted 4.10.0-rc6_VPHNt010+ #19
[    8.599475] task: c0000005f93c0001 task.stack: c000000bf8100000
[    8.599480] NIP: c00000000049c54c LR: c000000000101814 CTR: c0000000001190d0
[    8.599485] REGS: c000000bf8103520 TRAP: 0600   Not tainted  (4.10.0-rc6_VPHNt010+)
[    8.599490] MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE>
[    8.599495]   CR: 28108e44  XER: 0000000b
[    8.599501] CFAR: c000000000101810 DAR: c0000003c52231cf DSISR: 00000000 SOFTE: 0 
[    8.599501] GPR00: c0000000001017dc c000000bf81037a0 c000000000fd4c00 c0000005ef7c15a0 
[    8.599501] GPR04: c0000005ef7c15a0 c0000003c52231cf 0000000000000000 0000000000000000 
[    8.599501] GPR08: c000000001014c00 69665f716573006e fa00000000000000 0000000000000000 
[    8.599501] GPR12: c0000000001190d0 c00000000e5e3b00 c00000000000d718 0000000000000000 
[    8.599501] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
[    8.599501] GPR20: 0000000000000000 0000000000000000 c000000000c62b00 0000000000004000 
[    8.599501] GPR24: 00000003c45bfc57 c00000000100dbe0 0000000000000000 c000000000c62b00 
[    8.599501] GPR28: 00000003c45bfc57 c0000005ef7c1d4c 0000000000000800 c0000005ef7c1580 
[    8.634917] NIP [c00000000049c54c] llist_add_batch+0xc/0x40
[    8.634923] LR [c000000000101814] try_to_wake_up+0x3e4/0x500
[    8.634928] Call Trace:
[    8.634931] [c000000bf81037a0] [c0000000001017dc] try_to_wake_up+0x3ac/0x500 (unreliable)
[    8.634939] [c000000bf8103820] [c0000000000e5508] create_worker+0x148/0x250
[    8.679666] [c000000bf81038c0] [c0000000000e986c] alloc_unbound_pwq+0x3cc/0x4d0
[    8.679673] [c000000bf8103960] [c0000000000e9e9c] apply_wqattrs_prepare+0x2bc/0x330
[    8.679679] [c000000bf8103a10] [c0000000000e9f74] apply_workqueue_attrs_locked+0x64/0xd0
[    8.679685] [c000000bf8103a80] [c0000000000ea4f4] apply_workqueue_attrs+0x64/0xa0
[    8.679692] [c000000bf8103b00] [c0000000000ec19c] __alloc_workqueue_key+0x1cc/0x680
[    8.679700] [c000000bf8103be0] [c000000000bcb2a4] __machine_initcall_pseries_pseries_dlpar_init+0x50/0x8c
[    8.679707] [c000000bf8103c40] [c00000000000cef0] do_one_initcall+0x60/0x1c0
[    8.679714] [c000000bf8103d00] [c000000000bb423c] kernel_init_freeable+0x2a8/0x390
[    8.679719] [c000000bf8103dc0] [c00000000000d734] kernel_init+0x24/0x150
[    8.679726] [c000000bf8103e30] [c00000000000b4e8] ret_from_kernel_thread+0x5c/0x74
[    8.679731] Instruction dump:
[    8.706288] 60420000 7c832378 4e800020 60000000 60000000 60000000 60000000 60000000 
[    8.706296] 60000000 e9250000 f9240000 7c0004ac <7d4028a8> 7c2a4800 40c20010 7c6029ad 
[    8.706307] ---[ end trace b6256c8c7d38d99b ]---
[    8.706311] 
[   10.706438] Kernel panic - not syncing: Fatal exception
[   10.706704] Rebooting in 10 seconds..




-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web