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 <[email protected]>
---
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);
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;
--
2.43.0
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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.
--
tejun
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.
--
tejun
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