From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966973AbdEWTt4 (ORCPT ); Tue, 23 May 2017 15:49:56 -0400 Received: from mail-yw0-f196.google.com ([209.85.161.196]:33797 "EHLO mail-yw0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933846AbdEWTty (ORCPT ); Tue, 23 May 2017 15:49:54 -0400 Date: Tue, 23 May 2017 15:49:52 -0400 From: Tejun Heo To: Michael Bringmann Cc: Lai Jiangshan , linux-kernel@vger.kernel.org, Nathan Fontenot Subject: Re: [PATCH] workqueue: Ensure that cpumask set for pools created after boot Message-ID: <20170523194952.GF13222@htj.duckdns.org> References: <20170510173340.GF32165@htj.duckdns.org> <20170516155527.GB6389@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.2 (2017-04-18) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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