2014-04-03 14:43:02

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH 3/4] workqueue: Add anon workqueue sysfs hierarchy

On Sun, Mar 30, 2014 at 09:01:39AM -0400, Tejun Heo wrote:
> On Thu, Mar 27, 2014 at 06:21:01PM +0100, Frederic Weisbecker wrote:
> > We call "anon workqueues" the set of unbound workqueues that don't
> > carry the WQ_SYSFS flag.
> >
> > They are a problem nowadays because people who work on CPU isolation
> > (HPC, Real time, etc...) want to be able to migrate all the unbound
> > workqueues away to a single housekeeping CPU. This control is possible
> > through sysfs but only with WQ_SYSFS workqueues.
> >
> > Now we need to deal with the other unbound workqueues. There is two
> > possible solutions:
> >
> > 1) Implement a sysfs directory for each unbound !WQ_SYSFS. This could
> > be done with a specific Kconfig to make sure that these workqueue
> > won't be considered as a stable ABI. But we all know that all distros
> > will enable this Kconfig symbol and that a warning in the Kconfig help
> > text won't protect against anything.
> >
> > 2) Implement a single sysfs directory containing only the cpumask file
> > to the control the affinity of all the !WQ_SYSFS workqueues.
> >
> > This patch implements the second solution but only for non-ordered
> > unbound workqueues. Ordered workqueues need a special treatment and
> > will be handled in a subsequent patch.
>
> I'm not really sure this is the good approach. I think I wrote this
> way back but wouldn't it make more sense to allow userland to restrict
> the cpus which are allowed to all unbound cpus. As currently
> implemented, setting WQ_SYSFS to give userland more control would
> escape that workqueue from this global mask as a side effect, which is
> a surprising behavior and doesn't make much sense to me.

I just considered that anon workqueues shouldn't be that different from
another WQ_SYSFS workqueue. This way we don't have suprising side effect.
Touching a WQ_SYSFS doesn't impact anon workqueues, and touching anon workqueues
doesn't impact WQ_SYSFS workqueues.

In fact this is simply the current way we do it, just extended.

But anyway your solution looks more simple.

> I think it would make far more sense to implement a knob which controls which
> cpus are available to *all* unbound workqueue including the default
> fallback one. That way it's way more consistent and I'm pretty sure
> the code would be fairly simple too. All it needs to do is
> restricting the online cpus that unbound workqueues see.

Yeah I like this. So the right place for this cpumask would be in the root of
/sys/devices/virtual/workqueue/ , right?

Thanks.


2014-04-03 14:58:16

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH 3/4] workqueue: Add anon workqueue sysfs hierarchy

Hello, Frederic.

On Thu, Apr 03, 2014 at 04:42:55PM +0200, Frederic Weisbecker wrote:
> > I'm not really sure this is the good approach. I think I wrote this
> > way back but wouldn't it make more sense to allow userland to restrict
> > the cpus which are allowed to all unbound cpus. As currently
> > implemented, setting WQ_SYSFS to give userland more control would
> > escape that workqueue from this global mask as a side effect, which is
> > a surprising behavior and doesn't make much sense to me.
>
> I just considered that anon workqueues shouldn't be that different from
> another WQ_SYSFS workqueue. This way we don't have suprising side effect.
> Touching a WQ_SYSFS doesn't impact anon workqueues, and touching anon workqueues
> doesn't impact WQ_SYSFS workqueues.

I really think it'd be a lot better to perceive the default attributes
to be layered below explicit WQ_SYSFS attributes; otherwise, we have
two disjoint sets and the workqueues would jump between the two
depending on WQ_SYSFS. A system tool which wants to configure all
workqueues would have to scan and manipulate all of them not knowing
what specific one's requirements are and tools which want to configure
specific ones likely won't know what the overruling condition is and
violate the global contraints. It'd be clearer to have the layering
pre-defined and enforced.

> In fact this is simply the current way we do it, just extended.

Yes, in term of mechanics, it is but I don't think that's what we
want. We want to be able to say "unbound workqueues in general are
confined to these cpus" and it's weird to just provide knobs for wqs
which don't have knobs.

> Yeah I like this. So the right place for this cpumask would be in
> the root of /sys/devices/virtual/workqueue/ , right?

Yes, I think that'd make more sense.

Thanks.

--
tejun

2014-04-03 15:05:31

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH 3/4] workqueue: Add anon workqueue sysfs hierarchy

On Thu, Apr 03, 2014 at 10:58:05AM -0400, Tejun Heo wrote:
> Hello, Frederic.
>
> On Thu, Apr 03, 2014 at 04:42:55PM +0200, Frederic Weisbecker wrote:
> > > I'm not really sure this is the good approach. I think I wrote this
> > > way back but wouldn't it make more sense to allow userland to restrict
> > > the cpus which are allowed to all unbound cpus. As currently
> > > implemented, setting WQ_SYSFS to give userland more control would
> > > escape that workqueue from this global mask as a side effect, which is
> > > a surprising behavior and doesn't make much sense to me.
> >
> > I just considered that anon workqueues shouldn't be that different from
> > another WQ_SYSFS workqueue. This way we don't have suprising side effect.
> > Touching a WQ_SYSFS doesn't impact anon workqueues, and touching anon workqueues
> > doesn't impact WQ_SYSFS workqueues.
>
> I really think it'd be a lot better to perceive the default attributes
> to be layered below explicit WQ_SYSFS attributes; otherwise, we have
> two disjoint sets and the workqueues would jump between the two
> depending on WQ_SYSFS. A system tool which wants to configure all
> workqueues would have to scan and manipulate all of them not knowing
> what specific one's requirements are and tools which want to configure
> specific ones likely won't know what the overruling condition is and
> violate the global contraints. It'd be clearer to have the layering
> pre-defined and enforced.

Ok, works for me.

>
> > In fact this is simply the current way we do it, just extended.
>
> Yes, in term of mechanics, it is but I don't think that's what we
> want. We want to be able to say "unbound workqueues in general are
> confined to these cpus" and it's weird to just provide knobs for wqs
> which don't have knobs.

Yeah I like the simplicity of that.

>
> > Yeah I like this. So the right place for this cpumask would be in
> > the root of /sys/devices/virtual/workqueue/ , right?
>
> Yes, I think that'd make more sense.

Ok, I'll try this. Thanks!

>
> Thanks.
>
> --
> tejun