linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem.
@ 2016-01-07 12:38 wanghaibin
  2016-01-07 12:38 ` [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location wanghaibin
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: wanghaibin @ 2016-01-07 12:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: tj, jiangshanlai, peter.huangpeng, 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.
  2016-01-07 12:38 [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem wanghaibin
@ 2016-01-07 12:38 ` wanghaibin
  2016-01-07 15:48   ` Tejun Heo
  2016-01-07 12:38 ` [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed wanghaibin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: wanghaibin @ 2016-01-07 12:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: tj, jiangshanlai, peter.huangpeng, 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.
  2016-01-07 12:38 [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem wanghaibin
  2016-01-07 12:38 ` [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location wanghaibin
@ 2016-01-07 12:38 ` wanghaibin
  2016-01-07 15:50   ` Tejun Heo
  2016-01-07 12:38 ` [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device wanghaibin
  2016-01-07 12:38 ` [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function wanghaibin
  3 siblings, 1 reply; 12+ messages in thread
From: wanghaibin @ 2016-01-07 12:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: tj, jiangshanlai, peter.huangpeng, 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device
  2016-01-07 12:38 [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem wanghaibin
  2016-01-07 12:38 ` [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location wanghaibin
  2016-01-07 12:38 ` [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed wanghaibin
@ 2016-01-07 12:38 ` wanghaibin
  2016-01-07 16:00   ` Tejun Heo
  2016-01-07 12:38 ` [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function wanghaibin
  3 siblings, 1 reply; 12+ messages in thread
From: wanghaibin @ 2016-01-07 12:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: tj, jiangshanlai, peter.huangpeng, 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function.
  2016-01-07 12:38 [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem wanghaibin
                   ` (2 preceding siblings ...)
  2016-01-07 12:38 ` [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device wanghaibin
@ 2016-01-07 12:38 ` wanghaibin
  2016-01-07 16:07   ` Tejun Heo
  3 siblings, 1 reply; 12+ messages in thread
From: wanghaibin @ 2016-01-07 12:38 UTC (permalink / raw)
  To: linux-kernel; +Cc: tj, jiangshanlai, peter.huangpeng, 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.
  2016-01-07 12:38 ` [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location wanghaibin
@ 2016-01-07 15:48   ` Tejun Heo
  2016-01-11  5:21     ` wanghaibin
  0 siblings, 1 reply; 12+ messages in thread
From: Tejun Heo @ 2016-01-07 15:48 UTC (permalink / raw)
  To: wanghaibin; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.
  2016-01-07 12:38 ` [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed wanghaibin
@ 2016-01-07 15:50   ` Tejun Heo
  2016-01-11  4:26     ` wanghaibin
  0 siblings, 1 reply; 12+ messages in thread
From: Tejun Heo @ 2016-01-07 15:50 UTC (permalink / raw)
  To: wanghaibin; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device
  2016-01-07 12:38 ` [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device wanghaibin
@ 2016-01-07 16:00   ` Tejun Heo
  2016-01-11 12:23     ` wanghaibin
  0 siblings, 1 reply; 12+ messages in thread
From: Tejun Heo @ 2016-01-07 16:00 UTC (permalink / raw)
  To: wanghaibin; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function.
  2016-01-07 12:38 ` [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function wanghaibin
@ 2016-01-07 16:07   ` Tejun Heo
  0 siblings, 0 replies; 12+ messages in thread
From: Tejun Heo @ 2016-01-07 16:07 UTC (permalink / raw)
  To: wanghaibin; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed.
  2016-01-07 15:50   ` Tejun Heo
@ 2016-01-11  4:26     ` wanghaibin
  0 siblings, 0 replies; 12+ messages in thread
From: wanghaibin @ 2016-01-11  4:26 UTC (permalink / raw)
  To: Tejun Heo; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location.
  2016-01-07 15:48   ` Tejun Heo
@ 2016-01-11  5:21     ` wanghaibin
  0 siblings, 0 replies; 12+ messages in thread
From: wanghaibin @ 2016-01-11  5:21 UTC (permalink / raw)
  To: Tejun Heo; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device
  2016-01-07 16:00   ` Tejun Heo
@ 2016-01-11 12:23     ` wanghaibin
  0 siblings, 0 replies; 12+ messages in thread
From: wanghaibin @ 2016-01-11 12:23 UTC (permalink / raw)
  To: Tejun Heo; +Cc: linux-kernel, jiangshanlai, peter.huangpeng

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.

> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-01-11 12:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-07 12:38 [RFC PATCH 0/4] fixs/suggestions for workqueue subsystem wanghaibin
2016-01-07 12:38 ` [RFC PATCH 1/4] workqueue: move the wq_update_unbound_numa_attrs_buf allocation location wanghaibin
2016-01-07 15:48   ` Tejun Heo
2016-01-11  5:21     ` wanghaibin
2016-01-07 12:38 ` [RFC PATCH 2/4] workqueue: free the allocated memory resource when wq_numa_init failed wanghaibin
2016-01-07 15:50   ` Tejun Heo
2016-01-11  4:26     ` wanghaibin
2016-01-07 12:38 ` [RFC PATCH 3/4] workqueue: remove the unbind workqueue attr sys_file before unregister the workqueue device wanghaibin
2016-01-07 16:00   ` Tejun Heo
2016-01-11 12:23     ` wanghaibin
2016-01-07 12:38 ` [RFC PATCH 4/4] workqueue: simplify the apply_workqueue_attrs_locked function wanghaibin
2016-01-07 16:07   ` Tejun Heo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).