[RFC PATCH 0/4] fixs/suggestions for workqueue subsystem.

[RFC PATCH 0/4] fixs/suggestions for workqueue subsystem.

[wanghaibin]

Here are several fixs/suggestions about workqueue subsystem.

I am a newcomer for workqueue, and have no idea about workqueue test.
Do we have the better way (just like the test units)?
These patches are very simple, and just come from code reading :).

wanghaibin (4):
  workqueue: move the wq_update_unbound_numa_attrs_buf allocation
    location.
  workqueue: free the allocated memory resource when wq_numa_init
    failed.
  workqueue: remove the unbind workqueue attr sys_file before unregister
    the workqueue device
  workqueue: simplify the apply_workqueue_attrs_locked function.

 kernel/workqueue.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.

[wanghaibin]

the wq_update_unbound_numa_attrs_buf will be useful, only when the
wq_numa_enabled is true.
if there is something wrong to cause the wq_numa_enable false, it
can just return without the wq_update_unbound_numa_attrs_buf
allocation.

This doesn't introduce any functional changes.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 kernel/workqueue.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c579dba..d6cbe3d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5180,9 +5180,6 @@ static void __init wq_numa_init(void)
 		return;
 	}
 
-	wq_update_unbound_numa_attrs_buf = alloc_workqueue_attrs(GFP_KERNEL);
-	BUG_ON(!wq_update_unbound_numa_attrs_buf);
-
 	/*
 	 * We want masks of possible CPUs of each node which isn't readily
 	 * available.  Build one from cpu_to_node() which should have been
@@ -5207,6 +5204,9 @@ static void __init wq_numa_init(void)
 
 	wq_numa_possible_cpumask = tbl;
 	wq_numa_enabled = true;
+
+	wq_update_unbound_numa_attrs_buf = alloc_workqueue_attrs(GFP_KERNEL);
+	BUG_ON(!wq_update_unbound_numa_attrs_buf);
 }
 
 static int __init init_workqueues(void)
-- 
1.8.3.1

Re: [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.

[Tejun Heo]

On Thu, Jan 07, 2016 at 08:38:56PM +0800, wanghaibin wrote:
> the wq_update_unbound_numa_attrs_buf will be useful, only when the
> wq_numa_enabled is true.
> if there is something wrong to cause the wq_numa_enable false, it
> can just return without the wq_update_unbound_numa_attrs_buf
> allocation.
> 
> This doesn't introduce any functional changes.

I don't see what the point is with this change.

Thanks.

-- 
tejun

Re: [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.

[wanghaibin]

On 2016/1/7 23:48, Tejun Heo wrote:

> On Thu, Jan 07, 2016 at 08:38:56PM +0800, wanghaibin wrote:
>> the wq_update_unbound_numa_attrs_buf will be useful, only when the
>> wq_numa_enabled is true.
>> if there is something wrong to cause the wq_numa_enable false, it
>> can just return without the wq_update_unbound_numa_attrs_buf
>> allocation.
>>
>> This doesn't introduce any functional changes.
> 
> I don't see what the point is with this change.
> 

What I Meant To Say,  if (WARN_ON(node == NUMA_NO_NODE)) is true, this cause
wq_numa_enabled to be set the false.  That is, the wq_update_unbound_numa_attrs_buf
will be useless.

It can free the the wq_update_unbound_numa_attrs_buf while the WARN_ON condition is
true;
Or, better way is that only when the wq_numa_enabled is true, we will allocate the
wq_update_unbound_numa_attrs_buf;

However, just like your said, the WARN_ON condition should never happen,
Maybe this change is not useless too :) .

> Thanks.
> 

[RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.

[wanghaibin]

Before the return that wq_numa_init failed, it will be bettet to free
the numa node table to avoid the memory leaks.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 kernel/workqueue.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index d6cbe3d..60e192c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5195,6 +5195,10 @@ static void __init wq_numa_init(void)
 	for_each_possible_cpu(cpu) {
 		node = cpu_to_node(cpu);
 		if (WARN_ON(node == NUMA_NO_NODE)) {
+			for_each_node(node)
+				free_cpumask_var(tbl[node]);
+			kfree(tbl);
+
 			pr_warn("workqueue: NUMA node mapping not available for cpu%d, disabling NUMA support\n", cpu);
 			/* happens iff arch is bonkers, let's just proceed */
 			return;
-- 
1.8.3.1

Re: [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.

[Tejun Heo]

On Thu, Jan 07, 2016 at 08:38:57PM +0800, wanghaibin wrote:
> Before the return that wq_numa_init failed, it will be bettet to free
> the numa node table to avoid the memory leaks.

That's a WARN_ON condition which should never happen.  The system is
already pretty messed up and just limping along.  It really doesn't
matter whether some memory is cleaned up or not.  Let's keep it
simple.

Thanks.

-- 
tejun

Re: [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.

[wanghaibin]

On 2016/1/7 23:50, Tejun Heo wrote:

> On Thu, Jan 07, 2016 at 08:38:57PM +0800, wanghaibin wrote:
>> Before the return that wq_numa_init failed, it will be bettet to free
>> the numa node table to avoid the memory leaks.
> 

OK, I just expect that it can free the memory in all exception cases!

> That's a WARN_ON condition which should never happen.  The system is
> already pretty messed up and just limping along.  It really doesn't
> matter whether some memory is cleaned up or not.  Let's keep it
> simple.

OK, get it !


> 
> Thanks.
> 

[RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device

[wanghaibin]

for workqueue's flag with the WQ_SYSFS | WQ_UNBOUND, the wq_dev will create some sys_files for
unbound attrs, so, remove the unbind workqueue attrs sys_files before unregister the
workqueue device.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 kernel/workqueue.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 60e192c..d6527dc 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5131,10 +5131,13 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 
 	if (wq->flags & WQ_UNBOUND) {
 		struct device_attribute *attr;
+		int cnt = 0, i;
 
-		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++) {
+		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++, cnt++) {
 			ret = device_create_file(&wq_dev->dev, attr);
 			if (ret) {
+				for (attr = wq_sysfs_unbound_attrs, i = 0; i < cnt; attr++, i++)
+					device_remove_file(&wq_dev->dev, attr);
 				device_unregister(&wq_dev->dev);
 				wq->wq_dev = NULL;
 				return ret;
@@ -5160,6 +5163,11 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq)
 	if (!wq->wq_dev)
 		return;
 
+	if (wq->flags & WQ_UNBOUND) {
+		struct device_attribute *attr;
+		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++)
+			device_remove_file(&wq_dev->dev, attr);
+	}
 	wq->wq_dev = NULL;
 	device_unregister(&wq_dev->dev);
 }
-- 
1.8.3.1

Re: [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device

[Tejun Heo]

On Thu, Jan 07, 2016 at 08:38:58PM +0800, wanghaibin wrote:
> for workqueue's flag with the WQ_SYSFS | WQ_UNBOUND, the wq_dev will create some sys_files for
> unbound attrs, so, remove the unbind workqueue attrs sys_files before unregister the
> workqueue device.

File removal on sysfs is recursive, right?  Does explicitly removing
file make any difference?

-- 
tejun

Re: [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device

[wanghaibin]

On 2016/1/8 0:00, Tejun Heo wrote:

> On Thu, Jan 07, 2016 at 08:38:58PM +0800, wanghaibin wrote:
>> for workqueue's flag with the WQ_SYSFS | WQ_UNBOUND, the wq_dev will create some sys_files for
>> unbound attrs, so, remove the unbind workqueue attrs sys_files before unregister the
>> workqueue device.
> 
> File removal on sysfs is recursive, right?  Does explicitly removing
> file make any difference?

Yes, It has been removed already, I made a mistake.
Thanks.

> 

[RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function.

[wanghaibin]

if the apply_wqattrs_prepare function return NULL, in its implement, it has already
clear up the related resource, so, it can return failed directly, to avoid to call
the clean up function again.

this doesn't introduce any functional changes, just a suggestion.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 kernel/workqueue.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index d6527dc..9c9a27e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3601,7 +3601,6 @@ static int apply_workqueue_attrs_locked(struct workqueue_struct *wq,
 					const struct workqueue_attrs *attrs)
 {
 	struct apply_wqattrs_ctx *ctx;
-	int ret = -ENOMEM;
 
 	/* only unbound workqueues can change attributes */
 	if (WARN_ON(!(wq->flags & WQ_UNBOUND)))
@@ -3612,16 +3611,14 @@ static int apply_workqueue_attrs_locked(struct workqueue_struct *wq,
 		return -EINVAL;
 
 	ctx = apply_wqattrs_prepare(wq, attrs);
+	if (!ctx)
+		return -ENOMEM;
 
 	/* the ctx has been prepared successfully, let's commit it */
-	if (ctx) {
-		apply_wqattrs_commit(ctx);
-		ret = 0;
-	}
-
+	apply_wqattrs_commit(ctx);
 	apply_wqattrs_cleanup(ctx);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
1.8.3.1

[RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function.

[Tejun Heo]

Hello,

Applied to wq/for-4.5 with some adjustments to patch title
description.

Thanks.
------ 8< ------
>From 6201171e3b2c02992e62448636631a0dfe4e9d20 Mon Sep 17 00:00:00 2001
From: wanghaibin <wanghaibin.wang@huawei.com>
Date: Thu, 7 Jan 2016 20:38:59 +0800
Subject: [PATCH] workqueue: simplify the apply_workqueue_attrs_locked()

If the apply_wqattrs_prepare() returns NULL, it has already cleaned up
the related resources, so it can return directly and avoid calling the
clean up function again.

This doesn't introduce any functional changes.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1ecb588..61a0264 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3651,7 +3651,6 @@ static int apply_workqueue_attrs_locked(struct workqueue_struct *wq,
 					const struct workqueue_attrs *attrs)
 {
 	struct apply_wqattrs_ctx *ctx;
-	int ret = -ENOMEM;
 
 	/* only unbound workqueues can change attributes */
 	if (WARN_ON(!(wq->flags & WQ_UNBOUND)))
@@ -3662,16 +3661,14 @@ static int apply_workqueue_attrs_locked(struct workqueue_struct *wq,
 		return -EINVAL;
 
 	ctx = apply_wqattrs_prepare(wq, attrs);
+	if (!ctx)
+		return -ENOMEM;
 
 	/* the ctx has been prepared successfully, let's commit it */
-	if (ctx) {
-		apply_wqattrs_commit(ctx);
-		ret = 0;
-	}
-
+	apply_wqattrs_commit(ctx);
 	apply_wqattrs_cleanup(ctx);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.5.0