[patch]workqueue: copy attr with all fields.

[patch]workqueue: copy attr with all fields.

[Shaohua Li]

copy_workqueue_attrs() forgets to copy no_numa field.

Signed-off-by: Shaohua Li <shli@fuisonio.com>

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0b72e81..a988f54 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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 */

Re: [patch]workqueue: copy attr with all fields.

[Tejun Heo]

Hello, Shaohua.

On Mon, Jul 29, 2013 at 02:24:12PM +0800, Shaohua Li wrote:
> 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.

-- 
tejun

Re: [patch]workqueue: copy attr with all fields.

[Shaohua Li]

On Mon, Jul 29, 2013 at 07:14:01AM -0400, Tejun Heo wrote:
> Hello, Shaohua.
> 
> On Mon, Jul 29, 2013 at 02:24:12PM +0800, Shaohua Li wrote:
> > 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.

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

Re: [patch]workqueue: copy attr with all fields.

[Tejun Heo]

Hello,

On Mon, Jul 29, 2013 at 08:52:59PM +0800, Shaohua Li wrote:
> 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.

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

-- 
tejun

Re: [patch]workqueue: copy attr with all fields.

[Shaohua Li]

On Mon, Jul 29, 2013 at 11:35:23AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jul 29, 2013 at 08:52:59PM +0800, Shaohua Li wrote:
> > 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.
> 
> > 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?

alloc_workqueue_attrs() always zero the new allocated attr.

Thanks,
Shaohua

Re: [patch]workqueue: copy attr with all fields.

[Tejun Heo]

Hello, Shaohua.

On Tue, Jul 30, 2013 at 01:49:55PM +0800, Shaohua Li wrote:
> > 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.

-- 
tejun

Re: [patch]workqueue: copy attr with all fields.

[Shaohua Li]

On Tue, Jul 30, 2013 at 10:14:19AM -0400, Tejun Heo wrote:
> Hello, Shaohua.
> 
> On Tue, Jul 30, 2013 at 01:49:55PM +0800, Shaohua Li wrote:
> > > 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().

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

Re: [patch]workqueue: copy attr with all fields.

[Tejun Heo]

Hello,

On Wed, Jul 31, 2013 at 09:39:29AM +0800, Shaohua Li wrote:
> 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.

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

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

-- 
tejun

Re: [patch]workqueue: copy attr with all fields.

[Shaohua Li]

On Wed, Jul 31, 2013 at 06:27:52AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 31, 2013 at 09:39:29AM +0800, Shaohua Li wrote:
> > 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.
> 
> > 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.
> 
> > 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 <shli@fusionio.com>

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0b72e81..daf3f03 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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) {

[PATCH] workqueue: copy workqueue_attrs with all fields

[Tejun Heo]

Hello,

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

Thanks a lot!
---------- 8< -----------
>From 4fb228c35f4522f83abacaecc9838f0ab391a9b6 Mon Sep 17 00:00:00 2001
From: Shaohua Li <shli@kernel.org>
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 <shli@fusionio.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/workqueue.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 55f5f0a..cc5fa06 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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) {
-- 
1.8.3.1