[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZabRlEklmuqwFPj-@slm.duckdns.org>
Date: Tue, 16 Jan 2024 08:57:24 -1000
From: Tejun Heo <tj@...nel.org>
To: Juri Lelli <juri.lelli@...hat.com>
Cc: Lai Jiangshan <jiangshanlai@...il.com>,
Aaron Tomlin <atomlin@...mlin.com>,
Valentin Schneider <vschneid@...hat.com>,
Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 3/4] kernel/workqueue: Distinguish between general
unbound and WQ_SYSFS cpumask changes
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@...hat.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.
--
tejun
Powered by blists - more mailing lists