From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764920AbdEWUJP (ORCPT ); Tue, 23 May 2017 16:09:15 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:35508 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1758176AbdEWUJN (ORCPT ); Tue, 23 May 2017 16:09:13 -0400 Subject: Re: [PATCH] workqueue: Ensure that cpumask set for pools created after boot To: Tejun Heo References: <20170510173340.GF32165@htj.duckdns.org> <20170516155527.GB6389@htj.duckdns.org> <20170523194952.GF13222@htj.duckdns.org> Cc: Lai Jiangshan , linux-kernel@vger.kernel.org, Nathan Fontenot From: Michael Bringmann Organization: IBM Linux Technology Center Date: Tue, 23 May 2017 15:09:07 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170523194952.GF13222@htj.duckdns.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17052320-0044-0000-0000-00000336CA11 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007107; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000212; SDB=6.00864623; UDB=6.00429235; IPR=6.00644403; BA=6.00005369; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015555; XFM=3.00000015; UTC=2017-05-23 20:09:10 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17052320-0045-0000-0000-00000764D7C3 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-23_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705230102 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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