[RFC,3/4] kernel/workqueue: Distinguish between general unbound and WQ_SYSFS cpumask changes
Message ID | 20240116161929.232885-4-juri.lelli@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:42cf:b0:101:a8e8:374 with SMTP id q15csp372560dye; Tue, 16 Jan 2024 08:32:04 -0800 (PST) X-Google-Smtp-Source: AGHT+IEaDu0qNLSsENAFhyJfk0dHLswFKxjsYAjQoulErpnantY60Y9AZekj2tKg7KhYqVSV9aHD X-Received: by 2002:a05:6358:5157:b0:175:64c5:ca6a with SMTP id 23-20020a056358515700b0017564c5ca6amr5842074rwj.40.1705422723938; Tue, 16 Jan 2024 08:32:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1705422723; cv=none; d=google.com; s=arc-20160816; b=Yy9FjBXm3x7aeAUiZfO8xxKJgDiEf4CP8iEWaPCA2qqKjolZ8OZeQMicHJr2ix3TfY dxav0FRaCNY2+J9uS5CgPhJByyjcbFJ0wZ+K3Bk/dpbBzt+j3gtQwDQtBmh73pu5eaW/ YY1ojMDAm/KvGnFLFnKvAS1i47emrIducdQPwnA2otB5SNNHDxSd3JZgxjQOa1EfugeG H6+ubjfKaWXZyPsdPTNXDjLeUvUc/PiZ5aFa5Rk+Q+gTsCvw5NtZ645ZxQ2isdMN1or/ ArTqPuNuUYbpC07iB0Em2GdFiWgGqQbHTkjw8n6apM670z8RvHBaZlW5Ux3qBD3yvm66 R6ww== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=s4fKpZjH++WRt1yV+Gjohw4fqgWgBKev/a1YdUdJYkI=; fh=Nsf1u1rbcJryyiFo9sS/VsYZC3vmqvYt/oMU7uIxbeo=; b=Ouy5mv+0Bv2sjtzF20ANJ5lA0jKpYiJyhfGhvElFXl1svzxjcJfI8R3CKxHlZTefpZ Qz0Jrg4GB67+AMpZBYrlIOtVhLr8tM1qF3k8eiO35n/XG4vUbnkEiUo+q4hmjRkAK8wi g4ZzSh5ID4b3h1czknoujbGDpz9pb65v6VlEhBr+NEPeconJNW9QX1PBCUJvZZSrXoND 9lKD7efZBV/EJjuN7QwVpsVvw59RElwgNt2g5CEnRXMKPPxkBM5CjA76MAemZhFCIBSf +z94GDLsUIykzjwOtcTl6tDJujgB5E31wYaJJg8zWmWaPWSi2JI7vNhjdxXvxjqLeNZu IJsw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jBFXv5r8; spf=pass (google.com: domain of linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id y5-20020a17090aa40500b0028dafc233e6si11298686pjp.131.2024.01.16.08.32.03 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jan 2024 08:32:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jBFXv5r8; spf=pass (google.com: domain of linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-27567-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 629F6286EE5 for <ouuuleilei@gmail.com>; Tue, 16 Jan 2024 16:20:50 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 0FA281CD22; Tue, 16 Jan 2024 16:20:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="jBFXv5r8" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1F41C1CABA for <linux-kernel@vger.kernel.org>; Tue, 16 Jan 2024 16:20:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705422003; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=s4fKpZjH++WRt1yV+Gjohw4fqgWgBKev/a1YdUdJYkI=; b=jBFXv5r8MedtjrQPeaqHWEe9Vb/TkhenEG7x+7BodPLIPXvZQ/2MoPljCAn9rRueCrnaaM NNVcFc/qDJ6haExEC2tH8sJZNoQ7dzCKnrQYirMpFU/5OSTKi+gzXAcv/vLDLW3UXcYGzH gvCQps7e6RFTyBaBu/ukzuHEHiBfNxk= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-553-LmG8ySuHOSyhu9_A6-SC9Q-1; Tue, 16 Jan 2024 11:19:58 -0500 X-MC-Unique: LmG8ySuHOSyhu9_A6-SC9Q-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7831bc14ae3so1385270485a.2 for <linux-kernel@vger.kernel.org>; Tue, 16 Jan 2024 08:19:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705421998; x=1706026798; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=s4fKpZjH++WRt1yV+Gjohw4fqgWgBKev/a1YdUdJYkI=; b=Ntlcsmxq6fK2ImFMM9kNTlb9YicO0gG9746E0qftEaVDdsKUjYs3pAvSdAThtvJ0Lw OsXgSb9//0mpIgsfVumtHFT3Z6ZpwjtfNXhTBtJEx6jCktBJxuuDgu7Sf9SAP5OM1SLD peKeI5FJIOF8rtA2BoCG9yh18SGh6izz0kxZbgth2CrhjuVDTvERjVwO2aHgEzpCTKOi t2mx6fo7l6hTYdT0Gg4HzWSqpjSZ25GIbneISEwD9gS5mPKhfh0+n2BmyT9QP9JaVoOZ Oo28uHFtldJucmBkePc4a/3JYXuMX+1DToxdNM4c98AN52JUQpBhLLSPuARWi7GblSb7 zvBw== X-Gm-Message-State: AOJu0YzoZlLoVaUFaQlYXa/iTYiJhmDTFzu4yStDxQbQ0yUy16LuVhpR YDvKGX8HsZuMgmBQENivOU4Hz7a7otOc4CbAUnSHp1Eh66LuouKuCJ00UGhNbillYCYMC499Zz9 FtJP+bCoZ6vzBj2dHnTnMLH5GXcifPns2 X-Received: by 2002:a05:620a:4799:b0:783:6831:70e3 with SMTP id dt25-20020a05620a479900b00783683170e3mr1526837qkb.142.1705421998034; Tue, 16 Jan 2024 08:19:58 -0800 (PST) X-Received: by 2002:a05:620a:4799:b0:783:6831:70e3 with SMTP id dt25-20020a05620a479900b00783683170e3mr1526825qkb.142.1705421997769; Tue, 16 Jan 2024 08:19:57 -0800 (PST) Received: from localhost.localdomain ([151.29.130.8]) by smtp.gmail.com with ESMTPSA id fd10-20020a05622a4d0a00b00429d3257dd6sm3809166qtb.45.2024.01.16.08.19.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jan 2024 08:19:57 -0800 (PST) From: Juri Lelli <juri.lelli@redhat.com> To: Tejun Heo <tj@kernel.org> Cc: Lai Jiangshan <jiangshanlai@gmail.com>, Aaron Tomlin <atomlin@atomlin.com>, Valentin Schneider <vschneid@redhat.com>, Waiman Long <longman@redhat.com>, Juri Lelli <juri.lelli@redhat.com>, linux-kernel@vger.kernel.org Subject: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general unbound and WQ_SYSFS cpumask changes Date: Tue, 16 Jan 2024 17:19:28 +0100 Message-ID: <20240116161929.232885-4-juri.lelli@redhat.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240116161929.232885-1-juri.lelli@redhat.com> References: <20240116161929.232885-1-juri.lelli@redhat.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788265337817340155 X-GMAIL-MSGID: 1788265337817340155 |
Series |
Fix handling of rescuers affinity
|
|
Commit Message
Juri Lelli
Jan. 16, 2024, 4:19 p.m. UTC
Both general unbound cpumask and per-wq (WQ_SYSFS) cpumask changes end
up calling apply_wqattrs_prepare for preparing for the change, but this
doesn't work well for general unbound cpumask changes as current
implementation won't be looking at the new unbound_cpumask.
Fix the prepare phase for general unbound cpumask changes by checking
which type of attributes (general vs. WQ_SYSFS) are actually changing.
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
kernel/workqueue.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
Comments
Hello, Juri. On Tue, Jan 16, 2024 at 05:19:28PM +0100, Juri Lelli wrote: > Both general unbound cpumask and per-wq (WQ_SYSFS) cpumask changes end > up calling apply_wqattrs_prepare for preparing for the change, but this > doesn't work well for general unbound cpumask changes as current > implementation won't be looking at the new unbound_cpumask. > > Fix the prepare phase for general unbound cpumask changes by checking > which type of attributes (general vs. WQ_SYSFS) are actually changing. > > Signed-off-by: Juri Lelli <juri.lelli@redhat.com> > --- > kernel/workqueue.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index 3a1d5a67bd66a..2ef6573909070 100644 > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -4359,7 +4359,17 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > * it even if we don't use it immediately. > */ > copy_workqueue_attrs(new_attrs, attrs); > - wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); > + > + /* > + * Is the user changing the general unbound_cpumask or is this a > + * WQ_SYSFS cpumask change? > + */ > + if (attrs == wq->unbound_attrs) > + cpumask_copy(new_attrs->cpumask, unbound_cpumask); > + else > + wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); > + > + cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask); This looks rather hacky. Can you elaborate how the current code misbehaves with an example? For the unbound_cpumask update path, the intention is supplying the current ->attrs (wq user's request) and the new unbound_cpumask expecting wqattrs_actualize_cpumask() to do the right thing. Is the problem that you want to have access to the effective cpumask for the entire workqueue? If so, we don't want to do that with ctx->attrs as that's supposed to carry the user's requested configuration rather than the currently effective. We can just add a new field. > cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask); > ctx->dfl_pwq = alloc_unbound_pwq(wq, new_attrs); > if (!ctx->dfl_pwq) > @@ -4377,12 +4387,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > } > } > > - /* save the user configured attrs and sanitize it. */ > - copy_workqueue_attrs(new_attrs, attrs); > - cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask); > - cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask); > ctx->attrs = new_attrs; This part exists to store the user requested configuration rather than the applied effective configuration, so that e.g. later if the unbound_cpumask changes, we can apply the effective mask according to the user's latest requested configuration. I'm not sure why this is being dropped. Thanks.
On 16/01/24 08:57, Tejun Heo wrote: > Hello, Juri. > > On Tue, Jan 16, 2024 at 05:19:28PM +0100, Juri Lelli wrote: > > Both general unbound cpumask and per-wq (WQ_SYSFS) cpumask changes end > > up calling apply_wqattrs_prepare for preparing for the change, but this > > doesn't work well for general unbound cpumask changes as current > > implementation won't be looking at the new unbound_cpumask. > > > > Fix the prepare phase for general unbound cpumask changes by checking > > which type of attributes (general vs. WQ_SYSFS) are actually changing. > > > > Signed-off-by: Juri Lelli <juri.lelli@redhat.com> > > --- > > kernel/workqueue.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > > index 3a1d5a67bd66a..2ef6573909070 100644 > > --- a/kernel/workqueue.c > > +++ b/kernel/workqueue.c > > @@ -4359,7 +4359,17 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > > * it even if we don't use it immediately. > > */ > > copy_workqueue_attrs(new_attrs, attrs); > > - wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); > > + > > + /* > > + * Is the user changing the general unbound_cpumask or is this a > > + * WQ_SYSFS cpumask change? > > + */ > > + if (attrs == wq->unbound_attrs) > > + cpumask_copy(new_attrs->cpumask, unbound_cpumask); > > + else > > + wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); > > + > > + cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask); > > This looks rather hacky. Can you elaborate how the current code misbehaves > with an example? I was trying to address the fact that ordered unbound workqueues didn't seem to reflect unbound_cpumask changes, e.g. wq_unbound_cpumask=00000003 edac-poller ordered,E 0xffffffff 000000ff kworker/R-edac- 351 0xffffffff 000000ff vs. edac-poller ordered,E 00000003 kworker/R-edac- 349 00000003 with the patch applied. But honestly, I'm now also not convinced what I'm proposing is correct, so I'll need to think more about it. Can you please confirm though that ordered unbound workqueues are not "special" for some reason and we would like them to follow unbound_cpumask changes as normal ubound workqueues? Thanks, Juri
Hello, On Wed, Jan 17, 2024 at 02:06:08PM +0100, Juri Lelli wrote: > > This looks rather hacky. Can you elaborate how the current code misbehaves > > with an example? > > I was trying to address the fact that ordered unbound workqueues didn't > seem to reflect unbound_cpumask changes, e.g. > > wq_unbound_cpumask=00000003 > > edac-poller ordered,E 0xffffffff 000000ff kworker/R-edac- 351 0xffffffff 000000ff > > vs. > > edac-poller ordered,E 00000003 kworker/R-edac- 349 00000003 > > with the patch applied. But honestly, I'm now also not convinced what > I'm proposing is correct, so I'll need to think more about it. > > Can you please confirm though that ordered unbound workqueues are not > "special" for some reason and we would like them to follow > unbound_cpumask changes as normal ubound workqueues? They aren't special and should follow the normal unbound workqueue cpumask. Thanks.
On 1/17/24 12:12, Tejun Heo wrote: > Hello, > > On Wed, Jan 17, 2024 at 02:06:08PM +0100, Juri Lelli wrote: >>> This looks rather hacky. Can you elaborate how the current code misbehaves >>> with an example? >> I was trying to address the fact that ordered unbound workqueues didn't >> seem to reflect unbound_cpumask changes, e.g. >> >> wq_unbound_cpumask=00000003 >> >> edac-poller ordered,E 0xffffffff 000000ff kworker/R-edac- 351 0xffffffff 000000ff >> >> vs. >> >> edac-poller ordered,E 00000003 kworker/R-edac- 349 00000003 >> >> with the patch applied. But honestly, I'm now also not convinced what >> I'm proposing is correct, so I'll need to think more about it. >> >> Can you please confirm though that ordered unbound workqueues are not >> "special" for some reason and we would like them to follow >> unbound_cpumask changes as normal ubound workqueues? > They aren't special and should follow the normal unbound workqueue cpumask. My impression is that changing the workqueue cpumask of ordered unbound workqueue may break the ordering guarantee momentarily. I was planning to look into this further to see if that is true when I have time. If it is not a concern, we should certainly apply the global unbound cpumask change to those workqueues as well. Cheers, Longman
Hello, On Wed, Jan 17, 2024 at 02:32:34PM -0500, Waiman Long wrote: > My impression is that changing the workqueue cpumask of ordered unbound > workqueue may break the ordering guarantee momentarily. I was planning to Ah, you're right. Changing cpumask would require changing the dfl_pwq and that can introduce extra concurrency and break ordering and it's exempt from unbound_cpumask updates. We likely need to add a mechanism for updating ordered wq's so that the new pwq doesn't become until the previous one is drained. Thanks.
On 17/01/24 09:42, Tejun Heo wrote: > Hello, > > On Wed, Jan 17, 2024 at 02:32:34PM -0500, Waiman Long wrote: > > My impression is that changing the workqueue cpumask of ordered unbound > > workqueue may break the ordering guarantee momentarily. I was planning to > > Ah, you're right. Changing cpumask would require changing the dfl_pwq and > that can introduce extra concurrency and break ordering and it's exempt from > unbound_cpumask updates. We likely need to add a mechanism for updating > ordered wq's so that the new pwq doesn't become until the previous one is > drained. Thanks for the additional info! Guess I'll need to think more about this and possibly coordinate the effort with Waiman. Best, Juri
diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 3a1d5a67bd66a..2ef6573909070 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -4359,7 +4359,17 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, * it even if we don't use it immediately. */ copy_workqueue_attrs(new_attrs, attrs); - wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); + + /* + * Is the user changing the general unbound_cpumask or is this a + * WQ_SYSFS cpumask change? + */ + if (attrs == wq->unbound_attrs) + cpumask_copy(new_attrs->cpumask, unbound_cpumask); + else + wqattrs_actualize_cpumask(new_attrs, unbound_cpumask); + + cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask); cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask); ctx->dfl_pwq = alloc_unbound_pwq(wq, new_attrs); if (!ctx->dfl_pwq) @@ -4377,12 +4387,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, } } - /* save the user configured attrs and sanitize it. */ - copy_workqueue_attrs(new_attrs, attrs); - cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask); - cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask); ctx->attrs = new_attrs; - ctx->wq = wq; return ctx;