workqueue: Control cpu affinity of all unbound workqueues v2

A workqueue directory implements at least two files: max_active and per_cpu. Since thse are constant over WQ_SYSFS workqueues, they are

implemented as bus attributes.

kernel/workqueue.c +5 -8

index 193e977..4d230e3 100644
@@ -3133,7 +3133,6 @@ static ssize_t per_cpu_show(struct device *dev, struct device_attribute *attr,
 
        return scnprintf(buf, PAGE_SIZE, "%d\n", (bool)!(wq->flags & WQ_UNBOUND));
 }
-static DEVICE_ATTR_RO(per_cpu);
 
 static ssize_t max_active_show(struct device *dev,
                               struct device_attribute *attr, char *buf)

@@ -3156,14 +3155,12 @@ static ssize_t max_active_store(struct device *dev,
        workqueue_set_max_active(wq, val);
        return count;
 }
-static DEVICE_ATTR_RW(max_active);
 
-static struct attribute *wq_sysfs_attrs[] = {
-       &dev_attr_per_cpu.attr,
-       &dev_attr_max_active.attr,
-       NULL,
+static struct device_attribute wq_sysfs_default_attrs[] = {
+       __ATTR(per_cpu, 0444, per_cpu_show, NULL),
+       __ATTR(max_active, 0644, max_active_show, max_active_store),
+       __ATTR_NULL,
 };
-ATTRIBUTE_GROUPS(wq_sysfs);
 
 static ssize_t wq_pool_ids_show(struct device *dev,
                                struct device_attribute *attr, char *buf)

@@ -3313,7 +3310,6 @@ static struct device_attribute wq_sysfs_unbound_attrs[] = {
 
 static struct bus_type wq_subsys = {
        .name                           = "workqueue",
-       .dev_groups                     = wq_sysfs_groups,
 };
 
 static int __init wq_sysfs_init(void)

@@ -3346,6 +3342,7 @@ static void wq_device_release(struct device *dev)
  */
 int workqueue_sysfs_register(struct workqueue_struct *wq)
 {
+       struct device_attribute *attr;
        struct wq_device *wq_dev;
        int ret;
 

@@ -3379,8 +3376,16 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
                return ret;

kernel/workqueue.c +0 -1

                for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++) {
                        ret = device_create_file(&wq_dev->dev, attr);
--

1.8.3.1

Nothing much, just some nitpicks :)

The workqueues are all listed in a global list protected by a big mutex. And this big mutex is used in apply_workqueue_attrs() as well.

Now as we plan to implement a directory to control the cpumask of all non-ABI unbound workqueues, we want to be able to iterate over all unbound workqueues and call apply_workqueue_attrs() for each of them with the new cpumask.

But the risk for a deadlock is on the way: we need to iterate the list of workqueues under wq_pool_mutex. But then apply_workqueue_attrs() itself calls wq_pool_mutex.

Quoted 8 lines · Frederic Weisbecker

The workqueues are all listed in a global list protected by a big mutex. And this big mutex is used in apply_workqueue_attrs() as well.

Now as we plan to implement a directory to control the cpumask of all non-ABI unbound workqueues, we want to be able to iterate over all unbound workqueues and call apply_workqueue_attrs() for each of them with the new cpumask.

Quoted 3 lines

But the risk for a deadlock is on the way: we need to iterate the list of workqueues under wq_pool_mutex. But then apply_workqueue_attrs() itself calls wq_pool_mutex.

Wouldn't the right thing to do would be factoring out apply_workqueue_attrs_locked()? It's cleaner to block out addition of new workqueues while the masks are being updated anyway.

Quoted 3 lines

Wouldn't the right thing to do would be factoring out apply_workqueue_attrs_locked()? It's cleaner to block out addition of new workqueues while the masks are being updated anyway.

I'm not quite sure I get what you suggest. Do you mean have apply_workqueue_attrs_locked() calling apply_workqueue_attrs() under the lock on this patch?

Hello,

Quoted 7 lines · Frederic Weisbecker

Quoted 3 lines

Wouldn't the right thing to do would be factoring out apply_workqueue_attrs_locked()? It's cleaner to block out addition of new workqueues while the masks are being updated anyway.

I'm not quite sure I get what you suggest. Do you mean have apply_workqueue_attrs_locked() calling apply_workqueue_attrs() under the lock on this patch?

Not sure it still matters but I was suggesting that creating apply_workqueue_attrs_locked() which requires the caller to handle locking and making apply_workqueue_attrs() a wrapper which grabs and releases lock around it, and using the former in locked iteration would work. lol has this explanation made it any clearer or is it even worse now? :)

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.

Quoted 17 lines · Frederic Weisbecker

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.

Quoted 6 lines

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 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.

Quoted 6 lines

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.

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

Hello, Frederic.

Quoted 11 lines · Frederic Weisbecker

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Signature · 1 line

tejun

kernel/workqueue.c +18 -1

 
 static int __init wq_sysfs_init(void)
 {
-       return subsys_virtual_register(&wq_subsys, NULL);
+       struct device *anon_dev;
+       int ret;
+
+       ret = subsys_virtual_register(&wq_subsys, NULL);
+       if (ret < 0)
+               return ret;
+
+       mutex_lock(&wq_unbound_mutex);
+       cpumask_copy(&wq_anon_cpumask, cpu_possible_mask);
+       mutex_unlock(&wq_unbound_mutex);
+
+       anon_dev = kzalloc(sizeof(*anon_dev), GFP_KERNEL);
+       if (!anon_dev)
+               return -ENOMEM;
+
+       anon_dev->bus = &wq_subsys;
+       anon_dev->init_name = "anon_wqs";
+       anon_dev->release = device_release;

kernel/workqueue.c +6 -0

+               return ret;
+       }
+
+       kobject_uevent(&anon_dev->kobj, KOBJ_ADD);
+
+       return 0;
 }
 core_initcall(wq_sysfs_init);

Changing the cpu affinity of workqueues through the sysfs interface is done with apply_workqueue_attrs() by replacing the old pwqs of a workqueue with new ones tied to worker pools that are affine to the new cpumask.

We can't do that with ordered workqueues however because the serialization of their works is enforced by keeping a single worker running. Replacing it with a new pool of single worker may open a small race window where two workers could run concurrently during the pwq replacement. Eventually this can break the ordering guarantee.

So ordered workqueues get a special treatment here. Since they run a single pwq with a single pool containing a single worker that all ordered workqueue should share, we simply change this worker affinity to the new cpumask.

Also just in case this behaviour change a bit in the future and some ordered workqueues have their private worker, lets iterate the affinity change over all ordered workqueue pools. This way it works in both cases:
whether a single worker is shared among all ordered workqueue pools or some of them run their private one.

Suggested-by Tejun Heo <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Viresh Kumar <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/workqueue.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 12 deletions(-)

kernel/workqueue.c +5 -2

index 2863c39..18807c7 100644
@@ -3330,10 +3330,60 @@ static ssize_t wq_anon_cpumask_show(struct device *dev,
 	return written;
 }
 
-/* Must be called with wq_unbound_mutex held */
-static int wq_anon_cpumask_set(cpumask_var_t cpumask)
+static int wq_ordered_cpumask_set(struct workqueue_struct *wq, cpumask_var_t cpumask)
+{
+	struct pool_workqueue *pwq;
+	struct worker_pool *pool;
+	struct worker *worker;

kernel/workqueue.c +35 -10

+	}
+	mutex_unlock(&pool->manager_mutex);
+
+	if (!ret) {
+		mutex_lock(&wq->mutex);
+		cpumask_copy(wq->unbound_attrs->cpumask, cpumask);
+		mutex_unlock(&wq->mutex);
+	}
+
+	mutex_unlock(&wq_pool_mutex);
+
+	return ret;
+}
+
+static int wq_anon_cpumask_set(struct workqueue_struct *wq, cpumask_var_t cpumask)
 {
 	struct workqueue_attrs *attrs;
+	int ret;
+
+	attrs = wq_sysfs_prep_attrs(wq);
+	if (!attrs)
+		return -ENOMEM;
+
+	cpumask_copy(attrs->cpumask, cpumask);
+	ret = apply_workqueue_attrs(wq, attrs);
+	free_workqueue_attrs(attrs);
+
+	return ret;
+}
+
+/* Must be called with wq_unbound_mutex held */
+static int wq_anon_cpumask_set_all(cpumask_var_t cpumask)
+{
 	struct workqueue_struct *wq;
 	int ret;
 

@@ -3343,15 +3393,9 @@ static int wq_anon_cpumask_set(cpumask_var_t cpumask)
 			continue;
 		/* Ordered workqueues need specific treatment */
 		if (wq->flags & __WQ_ORDERED)
-			continue;
-
-		attrs = wq_sysfs_prep_attrs(wq);
-		if (!attrs)
-			return -ENOMEM;
-
-		cpumask_copy(attrs->cpumask, cpumask);
-		ret = apply_workqueue_attrs(wq, attrs);
-		free_workqueue_attrs(attrs);
+			ret = wq_ordered_cpumask_set(wq, cpumask);
+		else
+			ret = wq_anon_cpumask_set(wq, cpumask);
 		if (ret)
 			break;
 	}

@@ -3376,7 +3420,7 @@ static ssize_t wq_anon_cpumask_store(struct device *dev,
 	get_online_cpus();
 	if (cpumask_intersects(cpumask, cpu_online_mask)) {
 		mutex_lock(&wq_unbound_mutex);
-		ret = wq_anon_cpumask_set(cpumask);
+		ret = wq_anon_cpumask_set_all(cpumask);
 		if (!ret)
 			cpumask_copy(&wq_anon_cpumask, cpumask);
 		mutex_unlock(&wq_unbound_mutex);

Quoted 49 lines · Frederic Weisbecker

Changing the cpu affinity of workqueues through the sysfs interface is done with apply_workqueue_attrs() by replacing the old pwqs of a workqueue with new ones tied to worker pools that are affine to the new cpumask.

We can't do that with ordered workqueues however because the serialization of their works is enforced by keeping a single worker running. Replacing it with a new pool of single worker may open a small race window where two workers could run concurrently during the pwq replacement. Eventually this can break the ordering guarantee.

So ordered workqueues get a special treatment here. Since they run a single pwq with a single pool containing a single worker that all ordered workqueue should share, we simply change this worker affinity to the new cpumask.

Also just in case this behaviour change a bit in the future and some ordered workqueues have their private worker, lets iterate the affinity change over all ordered workqueue pools. This way it works in both cases:
whether a single worker is shared among all ordered workqueue pools or some of them run their private one.

Suggested-by Tejun Heo <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Viresh Kumar <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/workqueue.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 12 deletions(-)

kernel/workqueue.c +5 -2

index 2863c39..18807c7 100644
@@ -3330,10 +3330,60 @@ static ssize_t wq_anon_cpumask_show(struct device *dev,
 	return written;
 }
 
-/* Must be called with wq_unbound_mutex held */
-static int wq_anon_cpumask_set(cpumask_var_t cpumask)
+static int wq_ordered_cpumask_set(struct workqueue_struct *wq, cpumask_var_t cpumask)
+{
+	struct pool_workqueue *pwq;
+	struct worker_pool *pool;
+	struct worker *worker;

kernel/workqueue.c static ssize_t wq_anon_cpumask_show(struct device *dev, · line 3347

Quoted 10 lines

kernel/workqueue.c +10 -0

+	int ret;
+	int wi;
+
+	mutex_lock(&wq_pool_mutex);
+	pwq = list_first_entry(&wq->pwqs, typeof(*pwq), pwqs_node);
+	pool = pwq->pool;
+
+	mutex_lock(&pool->manager_mutex);
+	for_each_pool_worker(worker, wi, pool) {
+		/* CHECKME: Should we hold pool->lock here? */

Don't need to hold pool->lock which is a spinlock. pool->manager_mutex is enough to apply cpumask setting (but we only do it in cpuhotplug callbacks now).

kernel/workqueue.c static ssize_t wq_anon_cpumask_show(struct device *dev, · line 3348

Quoted 1 line

kernel/workqueue.c +1 -0

+		ret = set_cpus_allowed_ptr(worker->task, cpumask);

no.

even the pwq(of ordered wq) shares the pool with other unbound pwq. we shouldn't change the worker's attr.

Although we can change the ordered wq to use dedicated pool.
But if we do so, I would recommend to remove the ordered wq from workqueue.c totally and ask the callers use queue_kthread_work() instead.

(dedicated or any)pool requires two workers at least, and requires much more other resource.

kernel/workqueue.c static ssize_t wq_anon_cpumask_show(struct device *dev, · line 3353

Quoted 5 lines

kernel/workqueue.c +5 -0

+		if (ret)
+			break;
+	}
+	if (!ret) {
+		cpumask_copy(pool->attrs->cpumask, cpumask);

pool->attr should be constant (except initialization/destruction).

Quoted 36 lines

kernel/workqueue.c +31 -0

+	}
+	mutex_unlock(&pool->manager_mutex);
+
+	if (!ret) {
+		mutex_lock(&wq->mutex);
+		cpumask_copy(wq->unbound_attrs->cpumask, cpumask);
+		mutex_unlock(&wq->mutex);
+	}
+
+	mutex_unlock(&wq_pool_mutex);
+
+	return ret;
+}
+
+static int wq_anon_cpumask_set(struct workqueue_struct *wq, cpumask_var_t cpumask)
 {
 	struct workqueue_attrs *attrs;
+	int ret;
+
+	attrs = wq_sysfs_prep_attrs(wq);
+	if (!attrs)
+		return -ENOMEM;
+
+	cpumask_copy(attrs->cpumask, cpumask);
+	ret = apply_workqueue_attrs(wq, attrs);
+	free_workqueue_attrs(attrs);
+
+	return ret;
+}
+
+/* Must be called with wq_unbound_mutex held */
+static int wq_anon_cpumask_set_all(cpumask_var_t cpumask)
+{
 	struct workqueue_struct *wq;
 	int ret;
 

Quoted 4 lines

kernel/workqueue.c +0 -0

@@ -3343,15 +3393,9 @@ static int wq_anon_cpumask_set(cpumask_var_t cpumask)
 			continue;
 		/* Ordered workqueues need specific treatment */
 		if (wq->flags & __WQ_ORDERED)

kernel/workqueue.c static int wq_anon_cpumask_set(cpumask_var_t cpumask) · line 3396

Quoted 10 lines

kernel/workqueue.c +1 -9

-			continue;
-
-		attrs = wq_sysfs_prep_attrs(wq);
-		if (!attrs)
-			return -ENOMEM;
-
-		cpumask_copy(attrs->cpumask, cpumask);
-		ret = apply_workqueue_attrs(wq, attrs);
-		free_workqueue_attrs(attrs);
+			ret = wq_ordered_cpumask_set(wq, cpumask);

Quoted 6 lines

kernel/workqueue.c +2 -0

+		else
+			ret = wq_anon_cpumask_set(wq, cpumask);
 		if (ret)
 			break;
 	}

kernel/workqueue.c static ssize_t wq_anon_cpumask_store(struct device *dev, · line 3426

Quoted 8 lines

kernel/workqueue.c +1 -1

@@ -3376,7 +3420,7 @@ static ssize_t wq_anon_cpumask_store(struct device *dev,
 	get_online_cpus();
 	if (cpumask_intersects(cpumask, cpu_online_mask)) {
 		mutex_lock(&wq_unbound_mutex);
-		ret = wq_anon_cpumask_set(cpumask);
+		ret = wq_anon_cpumask_set_all(cpumask);
 		if (!ret)
 			cpumask_copy(&wq_anon_cpumask, cpumask);
 		mutex_unlock(&wq_unbound_mutex);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 03/31/2014 08:50 PM, Lai Jiangshan wrote:

Sorry, I'm wrong.
Tejun had told there is only one default worker pool for ordered workqueues.

It is true. But this pool may share with other non-ordered workqueues which maybe have WQ_SYSFS. we should split this pool into two pools. one for ordered wqs, one for the rest.

Quoted 17 lines

Quoted 17 lines

kernel/workqueue.c +3 -7

+/* Must be called with wq_unbound_mutex held */
+static int wq_anon_cpumask_set_all(cpumask_var_t cpumask)
+{
 	struct workqueue_struct *wq;
 	int ret;

@@ -3343,15 +3393,9 @@ static int wq_anon_cpumask_set(cpumask_var_t cpumask)
 			continue;
 		/* Ordered workqueues need specific treatment */
 		if (wq->flags & __WQ_ORDERED)
-			continue;
-
-		attrs = wq_sysfs_prep_attrs(wq);
-		if (!attrs)
-			return -ENOMEM;
-
-		cpumask_copy(attrs->cpumask, cpumask);

Quoted 3 lines

kernel/workqueue.c static int wq_anon_cpumask_set(cpumask_var_t cpumask) · line 3396

Quoted 3 lines

kernel/workqueue.c +1 -2

-		ret = apply_workqueue_attrs(wq, attrs);
-		free_workqueue_attrs(attrs);
+			ret = wq_ordered_cpumask_set(wq, cpumask);

After the pool is split. wq_ordered_cpumask_set() should only be called once. once is enough for all ordered wqs.

From 534f1df8a5a03427b0fc382150fbd34e05648a28 Mon Sep 17 00:00:00 2001

From 534f1df8a5a03427b0fc382150fbd34e05648a28 Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Tue, 15 Apr 2014 17:52:19 +0800
Subject: [PATCH] workqueue: allow changing attributions of ordered workqueue

Allow changing ordered workqueue's cpumask
Allow changing ordered workqueue's nice value
Allow registering ordered workqueue to SYSFS

Still disallow changing ordered workqueue's max_active which breaks ordered guarantee Still disallow changing ordered workqueue's no_numa which breaks ordered guarantee

Changing attributions will introduce new pwqs in a given workqueue, and old implement doesn't have any solution to handle multi-pwqs on ordered workqueues, so changing attributions for ordered workqueues is disallowed.

After I investigated several solutions which try to handle multi-pwqs on ordered workqueues, I found the solution which reuse max_active are the simplest.
In this solution, the new introduced pwq's max_active is kept as *ZERO* until it become the oldest pwq. Thus only one (the oldest) pwq is active, and ordered is guarantee.

This solution forces ordered on higher level while the non-reentrancy is also forced. so we don't need to queue any work to its previous pool. And we shouldn't do it. we must disallow any work repeatedly requeues itself back-to-back
which keeps the oldest pwq stay and make newest(if have) pwq starvation(non-active for ever).

Build test only.
This patch depends on previous patch:
workqueue: add __WQ_FREEZING and remove POOL_FREEZING

Frederic:
You can pick this patch to your updated patchset before TJ apply it.

Signed-off-by Lai Jiangshan <[email protected]>

So IUUC this is to allow registering ordered workqueue as WQ_SYSFS? But I think

we are going to follow Tejun's suggestion to have a low level cpumask which defines the limit for all unbound workqueues. This indeed tremendously simplifies everyting. I'll post the series soon.

But maybe I'm missing other requirements that are fixed by your patch?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From 534f1df8a5a03427b0fc382150fbd34e05648a28 Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Tue, 15 Apr 2014 17:52:19 +0800
Subject: [PATCH] workqueue: allow changing attributions of ordered workqueue

Allow changing ordered workqueue's cpumask
Allow changing ordered workqueue's nice value
Allow registering ordered workqueue to SYSFS

Still disallow changing ordered workqueue's max_active which breaks ordered guarantee Still disallow changing ordered workqueue's no_numa which breaks ordered guarantee

Changing attributions will introduce new pwqs in a given workqueue, and old implement doesn't have any solution to handle multi-pwqs on ordered workqueues, so changing attributions for ordered workqueues is disallowed.

After I investigated several solutions which try to handle multi-pwqs on ordered workqueues, I found the solution which reuse max_active are the simplest.
In this solution, the new introduced pwq's max_active is kept as *ZERO* until it become the oldest pwq.

Thus only one (the oldest) pwq is active, and ordered is guarantee.

This solution forces ordered on higher level while the non-reentrancy is also forced. so we don't need to queue any work to its previous pool. And we shouldn't do it. we must disallow any work repeatedly requeues itself back-to-back
which keeps the oldest pwq stay and make newest(if have) pwq starvation(non-active for ever).

Build test only.
This patch depends on previous patch:
workqueue: add __WQ_FREEZING and remove POOL_FREEZING

Frederic:
You can pick this patch to your updated patchset before TJ apply it.

Signed-off-by Lai Jiangshan <[email protected]>

---
 kernel/workqueue.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)

kernel/workqueue.c +0 -0

index 92c9ada..fadcc4a 100644

kernel/workqueue.c +11 -9

 	if (last_pool && last_pool != pwq->pool) {
 		struct worker *worker;
 

@@ -3179,6 +3185,10 @@ static ssize_t wq_numa_store(struct device *dev, struct device_attribute *attr,
 	struct workqueue_attrs *attrs;
 	int v, ret;
 
+	/* Creating per-node pwqs breaks ordering guarantee. Keep no_numa = 1 */
+	if (WARN_ON(wq->flags & __WQ_ORDERED))
+		return -EINVAL;
+
 	attrs = wq_sysfs_prep_attrs(wq);
 	if (!attrs)
 		return -ENOMEM;

@@ -3239,14 +3249,6 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 	struct wq_device *wq_dev;
 	int ret;
 
-	/*
-	 * Adjusting max_active or creating new pwqs by applyting
-	 * attributes breaks ordering guarantee.  Disallow exposing ordered
-	 * workqueues.
-	 */
-	if (WARN_ON(wq->flags & __WQ_ORDERED))
-		return -EINVAL;
-
 	wq->wq_dev = wq_dev = kzalloc(sizeof(*wq_dev), GFP_KERNEL);
 	if (!wq_dev)
 		return -ENOMEM;

@@ -3568,6 +3570,13 @@ static void rcu_free_pwq(struct rcu_head *rcu)
 			container_of(rcu, struct pool_workqueue, rcu));
 }
 
+static struct pool_workqueue *oldest_pwq(struct workqueue_struct *wq)
+{
+	return list_last_entry(&wq->pwqs, struct pool_workqueue, pwqs_node);
+}
+
+static void pwq_adjust_max_active(struct pool_workqueue *pwq);
+
 /*
  * Scheduled on system_wq by put_pwq() when an unbound pwq hits zero refcnt
  * and needs to be destroyed.

@@ -3583,14 +3592,12 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
 	if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
 		return;
 
-	/*

kernel/workqueue.c +10 -0

 	mutex_unlock(&wq->mutex);
 
 	mutex_lock(&wq_pool_mutex);

@@ -3609,6 +3616,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
 	}
 }
 
+static bool pwq_active(struct pool_workqueue *pwq)
+{
+	/* Only the oldest pwq is active in the ordered wq */
+	if (pwq->wq->flags & __WQ_ORDERED)
+		return pwq == oldest_pwq(pwq->wq);
+
+	/* All pwqs in the non-ordered wq are active */
+	return true;
+}
+
 /**
  * pwq_adjust_max_active - update a pwq's max_active to the current setting
  * @pwq: target pool_workqueue

kernel/workqueue.c +6 -6

 		pwq->max_active = wq->saved_max_active;
 
 		while (!list_empty(&pwq->delayed_works) &&

@@ -3680,11 +3697,11 @@ static void link_pwq(struct pool_workqueue *pwq)
 	 */
 	pwq->work_color = wq->work_color;
 
+	/* link in @pwq on the head of &wq->pwqs */
+	list_add_rcu(&pwq->pwqs_node, &wq->pwqs);
+
 	/* sync max_active to the current setting */
 	pwq_adjust_max_active(pwq);
-
-	/* link in @pwq */
-	list_add_rcu(&pwq->pwqs_node, &wq->pwqs);
 }
 
 /* obtain a pool matching @attr and create a pwq associating the pool and @wq */

@@ -3810,8 +3827,8 @@ int apply_workqueue_attrs(struct workqueue_struct *wq,
 	if (WARN_ON(!(wq->flags & WQ_UNBOUND)))
 		return -EINVAL;
 
-	/* creating multiple pwqs breaks ordering guarantee */
-	if (WARN_ON((wq->flags & __WQ_ORDERED) && !list_empty(&wq->pwqs)))
+	/* creating multiple per-node pwqs breaks ordering guarantee */
+	if (WARN_ON((wq->flags & __WQ_ORDERED) && !attrs->no_numa))
 		return -EINVAL;
 
 	pwq_tbl = kzalloc(wq_numa_tbl_len * sizeof(pwq_tbl[0]), GFP_KERNEL);

@@ -4004,7 +4021,7 @@ out_unlock:
 static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 {
 	bool highpri = wq->flags & WQ_HIGHPRI;
-	int cpu, ret;
+	int cpu;
 
 	if (!(wq->flags & WQ_UNBOUND)) {
 		wq->cpu_pwqs = alloc_percpu(struct pool_workqueue);

@@ -4025,12 +4042,7 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 		}
 		return 0;
 	} else if (wq->flags & __WQ_ORDERED) {

Signature · 1 line

1.7.4.4

Hi Lai,

So actually I'll need to use apply_workqueue_attr() on the next patchset. So I'm considering this patch.

Some comments below:

Quoted 16 lines · Lai Jiangshan

From 534f1df8a5a03427b0fc382150fbd34e05648a28 Mon Sep 17 00:00:00 2001
From: Lai Jiangshan <[email protected]>
Date: Tue, 15 Apr 2014 17:52:19 +0800
Subject: [PATCH] workqueue: allow changing attributions of ordered workqueue

Allow changing ordered workqueue's cpumask
Allow changing ordered workqueue's nice value
Allow registering ordered workqueue to SYSFS

Still disallow changing ordered workqueue's max_active which breaks ordered guarantee Still disallow changing ordered workqueue's no_numa which breaks ordered guarantee

Changing attributions will introduce new pwqs in a given workqueue, and old implement doesn't have any solution to handle multi-pwqs on ordered workqueues, so changing attributions for ordered workqueues is disallowed.

Quoted 4 lines

After I investigated several solutions which try to handle multi-pwqs on ordered workqueues, I found the solution which reuse max_active are the simplest.
In this solution, the new introduced pwq's max_active is kept as *ZERO* until it become the oldest pwq.

I don't see where this zero value is enforced. Do you mean 1? That's the initial value of ordered max_active pools.

Quoted 31 lines

Thus only one (the oldest) pwq is active, and ordered is guarantee.

This solution forces ordered on higher level while the non-reentrancy is also forced. so we don't need to queue any work to its previous pool. And we shouldn't do it. we must disallow any work repeatedly requeues itself back-to-back
which keeps the oldest pwq stay and make newest(if have) pwq starvation(non-active for ever).

Build test only.
This patch depends on previous patch:
workqueue: add __WQ_FREEZING and remove POOL_FREEZING

Frederic:
You can pick this patch to your updated patchset before TJ apply it.

Signed-off-by Lai Jiangshan <[email protected]>

---
 kernel/workqueue.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)

kernel/workqueue.c +2 -0

index 92c9ada..fadcc4a 100644
@@ -1350,8 +1350,14 @@ retry:
 	 * If @work was previously on a different pool, it might still be
 	 * running there, in which case the work needs to be queued on that
 	 * pool to guarantee non-reentrancy.
+	 *
+	 * pwqs are guaranteed active orderly for ordered workqueue, and

kernel/workqueue.c retry: · line 1358

Quoted 4 lines

kernel/workqueue.c +4 -0

+	 * it guarantees non-reentrancy for works. So any work doesn't need
+	 * to be queued on previous pool. And the works shouldn't be queued
+	 * on previous pool, since we need to guarantee the prevous pwq
+	 * releasable to avoid work-stavation on the newest pool.

BTW, who needs this reentrancy guarantee? Is this an implicit guarantee for all workqueues that is there for backward compatibility? I've seen some patches dealing with that lately but I don't recall the details.

kernel/workqueue.c retry: · line 1360

Quoted 3 lines

kernel/workqueue.c +1 -1

 	 */
-	last_pool = get_work_pool(work);
+	last_pool = wq->flags & __WQ_ORDERED ? NULL : get_work_pool(work);

Does it hurt performance to do this old worker recovery? It seems to when I look at get_work_pool() and find_worker_executing_pool().

Perhaps we can generalize this check to all wqs which have only one worker?

Anyway that's just optimizations. Nothing that needs to be done in this patch.

So I have mixed feelings with this patch given the complication. But it's probably better to take that direction.

I just wish we had some way to automatically detect situations where a work mistakenly runs through re-entrancy. Because if it ever happens randomly, it's going to be a hell to debug.

Thanks.

Quoted 3 lines

Signature · 1 line

1.7.4.4

.