workqueue: copy attr with all fields.

PATCH In review Shaohua Li 2 participants 10 messages Jul 29, 2013 – Aug 1, 2013 Workqueue

lore

Share & tools

lore

Shaohua Li

author

2013-07-29 06:24 UTC #0 lore raw

PATCH 1/1 workqueue: copy attr with all fields.

copy_workqueue_attrs() forgets to copy no_numa field.

Signed-off-by Shaohua Li <[email protected]>

kernel/workqueue.c +1 -0

index 0b72e81..a988f54 100644
@@ -3411,6 +3411,7 @@ static void copy_workqueue_attrs(struct workqueue_attrs *to,
 {
 	to->nice = from->nice;
 	cpumask_copy(to->cpumask, from->cpumask);
+	to->no_numa = from->no_numa;
 }
 
 /* hash value of the content of @attr */

Tejun Heo

maintainer

2013-07-29 11:14 UTC #1 lore raw

Hello, Shaohua.

Quoted 1 line · Shaohua Li

copy_workqueue_attrs() forgets to copy no_numa field.

Explaining how that affects things and how the behavior changes afterwards would be nice. As it currently stands, I don't know why you're submitting this patch. Is this a bug fix? Just a consistency update? Should it be marked with -stable?

Please put more effort into patch descriptions and also note that ->no_numa isn't a pool attribute as noted in the comment above struct workqueue_attrs definition.

Thanks.

Signature · 1 line

tejun

Shaohua Li

author

2013-07-29 12:53 UTC #2 lore raw

Quoted 5 lines · Tejun Heo

Hello, Shaohua.
Quoted 1 line · Shaohua Li
copy_workqueue_attrs() forgets to copy no_numa field.

Quoted 8 lines

Explaining how that affects things and how the behavior changes afterwards would be nice. As it currently stands, I don't know why you're submitting this patch. Is this a bug fix? Just a consistency update? Should it be marked with -stable?
Please put more effort into patch descriptions and also note that ->no_numa isn't a pool attribute as noted in the comment above struct workqueue_attrs definition.

Ah, ok, I thought it's straightforward... It's a bug fix. echo '0' > /sys/bus/workqueue/devices/xxx/numa
cat /sys/bus/workqueue/devices/xxx/numa
I got 1. It should be 0, the reason is copy_workqueue_attrs() called in apply_workqueue_attrs() doesn't copy no_numa field.

Thanks,
Shaohua

Tejun Heo

maintainer

2013-07-29 15:35 UTC #3 lore raw

Hello,

Quoted 1 line · Shaohua Li

Ah, ok, I thought it's straightforward... It's a bug fix.

It never is, seriously. If it's something trivial / straighforward, please say so in the patch description because not doing so forces other people to guess the intention behind the change which sucks and this isn't even that trivial.

Quoted 4 lines

echo '0' > /sys/bus/workqueue/devices/xxx/numa
cat /sys/bus/workqueue/devices/xxx/numa
I got 1. It should be 0, the reason is copy_workqueue_attrs() called in apply_workqueue_attrs() doesn't copy no_numa field.

Note that copy_workqueue_attrs() is also used while creating worker pools and this change would mean that there will be worker pools with no_numa attribute assigned basically randomly depending on which wq created the pool for the first time which should be fine AFAICS but isn't very nice. Given that there's only single place where a pool's attrs is set, maybe it'd be best to clear no_numa right after copy and explain what's going on there?

Thanks.

Signature · 1 line

tejun

Shaohua Li

author

2013-07-30 05:50 UTC #4 lore raw

Quoted 15 lines · Tejun Heo

Hello,
Quoted 1 line · Shaohua Li
Ah, ok, I thought it's straightforward... It's a bug fix.
It never is, seriously. If it's something trivial / straighforward, please say so in the patch description because not doing so forces other people to guess the intention behind the change which sucks and this isn't even that trivial.
Quoted 4 lines
echo '0' > /sys/bus/workqueue/devices/xxx/numa
cat /sys/bus/workqueue/devices/xxx/numa
I got 1. It should be 0, the reason is copy_workqueue_attrs() called in apply_workqueue_attrs() doesn't copy no_numa field.

Quoted 7 lines

Note that copy_workqueue_attrs() is also used while creating worker pools and this change would mean that there will be worker pools with no_numa attribute assigned basically randomly depending on which wq created the pool for the first time which should be fine AFAICS but isn't very nice. Given that there's only single place where a pool's attrs is set, maybe it'd be best to clear no_numa right after copy and explain what's going on there?

alloc_workqueue_attrs() always zero the new allocated attr.

Thanks,
Shaohua

Tejun Heo

maintainer

2013-07-30 14:14 UTC #5 lore raw

Hello, Shaohua.

Quoted 9 lines · Shaohua Li

Quoted 7 lines
Note that copy_workqueue_attrs() is also used while creating worker pools and this change would mean that there will be worker pools with no_numa attribute assigned basically randomly depending on which wq created the pool for the first time which should be fine AFAICS but isn't very nice. Given that there's only single place where a pool's attrs is set, maybe it'd be best to clear no_numa right after copy and explain what's going on there?
alloc_workqueue_attrs() always zero the new allocated attr.

Hmmm? Yeah, sure, that's how it currently is always zero for all pools. If you change the copy function, it'll now be set to whatever the input value was when a pool was created, which shouldn't break anything but is a bit confusing as no_numa doesn't have anything to do with pools directly. Just in case, I'm talking about copy_workqueue_attrs() call in get_unbound_pool().

Thanks.

Signature · 1 line

tejun

Shaohua Li

author

2013-07-31 01:39 UTC #6 lore raw

Quoted 11 lines · Tejun Heo

Hello, Shaohua.
Quoted 8 lines · Shaohua Li
Quoted 7 lines
Note that copy_workqueue_attrs() is also used while creating worker pools and this change would mean that there will be worker pools with no_numa attribute assigned basically randomly depending on which wq created the pool for the first time which should be fine AFAICS but isn't very nice. Given that there's only single place where a pool's attrs is set, maybe it'd be best to clear no_numa right after copy and explain what's going on there?

Quoted 8 lines

Quoted 1 line
alloc_workqueue_attrs() always zero the new allocated attr.
Hmmm? Yeah, sure, that's how it currently is always zero for all pools. If you change the copy function, it'll now be set to whatever the input value was when a pool was created, which shouldn't break anything but is a bit confusing as no_numa doesn't have anything to do with pools directly. Just in case, I'm talking about copy_workqueue_attrs() call in get_unbound_pool().

Hmm, I didn't agree it's more confusing to change copy_workqueue_attrs(), the name of the function suggests it is a 'copy'. And clearing no_numa in apply_workqueue_attrs() after copy_workqueue_attrs() looks like a hack to me. But it depends on you, feel free to fix it by yourself.

Thanks,
Shaohua

Tejun Heo

maintainer

2013-07-31 10:27 UTC #7 lore raw

Hello,

Quoted 1 line · Shaohua Li

Hmm, I didn't agree it's more confusing to change copy_workqueue_attrs(), the

We're talking past each other. I'm not saying copy_workqueue_attrs() shouldn't copy no_numa. I'm saying get_unbound_pool() should clear no_numa so that pools don't have random no_numa settings.

Quoted 2 lines

name of the function suggests it is a 'copy'. And clearing no_numa in apply_workqueue_attrs() after copy_workqueue_attrs() looks like a hack to me.

Why would apply_workqueue_attrs() modify no_numa when it *is* dealing with an actual workqueue attr. I've been talking about get_unbound_pool() not apply_workqueue_attrs() the whole time.

Quoted 1 line

But it depends on you, feel free to fix it by yourself.

Please update the patch to add no_numa clearing to get_unbound_pool() and explain what's going on.

Thanks.

Signature · 1 line

tejun

Shaohua Li

author

2013-08-01 01:56 UTC #8 lore raw

Quoted 12 lines · Tejun Heo

Hello,
Quoted 1 line · Shaohua Li
Hmm, I didn't agree it's more confusing to change copy_workqueue_attrs(), the
We're talking past each other. I'm not saying copy_workqueue_attrs() shouldn't copy no_numa. I'm saying get_unbound_pool() should clear no_numa so that pools don't have random no_numa settings.
Quoted 2 lines
name of the function suggests it is a 'copy'. And clearing no_numa in apply_workqueue_attrs() after copy_workqueue_attrs() looks like a hack to me.

Quoted 8 lines

Why would apply_workqueue_attrs() modify no_numa when it *is* dealing with an actual workqueue attr. I've been talking about get_unbound_pool() not apply_workqueue_attrs() the whole time.
Quoted 1 line
But it depends on you, feel free to fix it by yourself.
Please update the patch to add no_numa clearing to get_unbound_pool() and explain what's going on.

Something like this?

Subject: workqueue: copy workqueue_attrs with all fields

$echo '0' > /sys/bus/workqueue/devices/xxx/numa
$cat /sys/bus/workqueue/devices/xxx/numa
I got 1. It should be 0, the reason is copy_workqueue_attrs() called in apply_workqueue_attrs() doesn't copy no_numa field.

Signed-off-by Shaohua Li <[email protected]>

kernel/workqueue.c +7 -0

index 0b72e81..daf3f03 100644
@@ -3411,6 +3411,7 @@ static void copy_workqueue_attrs(struct workqueue_attrs *to,
 {
 	to->nice = from->nice;
 	cpumask_copy(to->cpumask, from->cpumask);
+	to->no_numa = from->no_numa;
 }
 
 /* hash value of the content of @attr */

@@ -3578,6 +3579,12 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
 	lockdep_set_subclass(&pool->lock, 1);	/* see put_pwq() */
 	copy_workqueue_attrs(pool->attrs, attrs);
 
+	/*
+	 * no_numa isn't a worker_pool attribute, always clear it. See 'struct
+	 * workqueue_attrs' comments for detail.
+	 */
+	pool->attrs->no_numa = 0;
+
 	/* if cpumask is contained inside a NUMA node, we belong to that node */
 	if (wq_numa_enabled) {
 		for_each_node(node) {

Tejun Heo

maintainer

2013-08-01 12:35 UTC #9 lore raw

PATCH 1/1 workqueue: copy workqueue_attrs with all fields

Hello,

Applied w/ some doc updates to wq/for-3.11f-fixes.

Thanks a lot!
---------- 8< -----------

Quoted 1 line

From 4fb228c35f4522f83abacaecc9838f0ab391a9b6 Mon Sep 17 00:00:00 2001

From: Shaohua Li <[email protected]>
Date: Thu, 1 Aug 2013 09:56:36 +0800

$echo '0' > /sys/bus/workqueue/devices/xxx/numa
$cat /sys/bus/workqueue/devices/xxx/numa

I got 1. It should be 0, the reason is copy_workqueue_attrs() called in apply_workqueue_attrs() doesn't copy no_numa field.

Fix it by making copy_workqueue_attrs() copy ->no_numa too. This would also make get_unbound_pool() set a pool's ->no_numa attribute according to the workqueue attributes used when the pool was created. While harmelss, as ->no_numa isn't a pool attribute, this is a bit confusing. Clear it explicitly.

tj: Updated description and comments a bit.

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by Tejun Heo <[email protected]>

Cc [email protected]

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

kernel/workqueue.c +12 -0

index 55f5f0a..cc5fa06 100644
@@ -3416,6 +3416,12 @@ static void copy_workqueue_attrs(struct workqueue_attrs *to,
 {
 	to->nice = from->nice;
 	cpumask_copy(to->cpumask, from->cpumask);
+	/*
+	 * Unlike hash and equality test, this function doesn't ignore
+	 * ->no_numa as it is used for both pool and wq attrs.  Instead,
+	 * get_unbound_pool() explicitly clears ->no_numa after copying.
+	 */
+	to->no_numa = from->no_numa;
 }
 
 /* hash value of the content of @attr */

@@ -3583,6 +3589,12 @@ static struct worker_pool *get_unbound_pool(const struct workqueue_attrs *attrs)
 	lockdep_set_subclass(&pool->lock, 1);	/* see put_pwq() */
 	copy_workqueue_attrs(pool->attrs, attrs);
 
+	/*
+	 * no_numa isn't a worker_pool attribute, always clear it.  See
+	 * 'struct workqueue_attrs' comments for detail.
+	 */
+	pool->attrs->no_numa = 0;
+
 	/* if cpumask is contained inside a NUMA node, we belong to that node */
 	if (wq_numa_enabled) {
 		for_each_node(node) {

Signature · 1 line

1.8.3.1