[PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init()

[PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init()

[Chen Gang]

When NUMA node mapping not available for one cpu, need disabling NUMA
support and proceed.

Also better to release unused resources (the original code can not
cause issue, only waste some memory).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/workqueue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1ae6028..a763079 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4911,6 +4911,9 @@ static void __init wq_numa_init(void)
 		if (WARN_ON(node == NUMA_NO_NODE)) {
 			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 */
+			for_each_node(node)
+				free_cpumask_var(tbl[node]);
+			kfree(tbl);
 			return;
 		}
 		cpumask_set_cpu(cpu, tbl[node]);
-- 
1.7.11.7

Re: [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init()

[Tejun Heo]

On Tue, May 14, 2013 at 08:25:20PM +0800, Chen Gang wrote:
> 
> When NUMA node mapping not available for one cpu, need disabling NUMA
> support and proceed.
> 
> Also better to release unused resources (the original code can not
> cause issue, only waste some memory).
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/workqueue.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 1ae6028..a763079 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4911,6 +4911,9 @@ static void __init wq_numa_init(void)
>  		if (WARN_ON(node == NUMA_NO_NODE)) {
>  			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 */
> +			for_each_node(node)
> +				free_cpumask_var(tbl[node]);
> +			kfree(tbl);

Eh... let's just leave it alone.  The path is taken iff the whole
thing is seriously broken and we're triggering WARN.  I don't think we
need to worry about small memory wastage there.

Thanks.

-- 
tejun

Re: [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init()

[Chen Gang]

On 05/14/2013 11:17 PM, Tejun Heo wrote:
> On Tue, May 14, 2013 at 08:25:20PM +0800, Chen Gang wrote:
>>
>> When NUMA node mapping not available for one cpu, need disabling NUMA
>> support and proceed.
>>
>> Also better to release unused resources (the original code can not
>> cause issue, only waste some memory).
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/workqueue.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 1ae6028..a763079 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -4911,6 +4911,9 @@ static void __init wq_numa_init(void)
>>  		if (WARN_ON(node == NUMA_NO_NODE)) {
>>  			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 */
>> +			for_each_node(node)
>> +				free_cpumask_var(tbl[node]);
>> +			kfree(tbl);
> 
> Eh... let's just leave it alone.  The path is taken iff the whole
> thing is seriously broken and we're triggering WARN.  I don't think we
> need to worry about small memory wastage there.
> 
> Thanks.
> 

OK, I can understand. I will continue to try to find and make another
patches.


Thanks.
-- 
Chen Gang

Asianux Corporation

[PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Chen Gang]

In workqueue_sysfs_register(), when failure occurs after called
device_create_file(), need call device_remove_file() to release the
related resources, then call device_unregister().

Or it will cause issue.

For individual 'device_attributs' (just like our case), need call
device_remove_file() explictly and then call device_unregister(),
please reference drivers/base/*.c (e.g node.c or cpu.c).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/workqueue.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1ae6028..de11dae 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3323,6 +3323,8 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++) {
 			ret = device_create_file(&wq_dev->dev, attr);
 			if (ret) {
+				while (--attr >= wq_sysfs_unbound_attrs)
+					device_remove_file(&wq_dev->dev, attr);
 				device_unregister(&wq_dev->dev);
 				wq->wq_dev = NULL;
 				return ret;
-- 
1.7.7.6

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Tejun Heo]

On Wed, May 15, 2013 at 02:13:14PM +0800, Chen Gang wrote:
> 
> In workqueue_sysfs_register(), when failure occurs after called
> device_create_file(), need call device_remove_file() to release the
> related resources, then call device_unregister().
> 
> Or it will cause issue.
> 
> For individual 'device_attributs' (just like our case), need call
> device_remove_file() explictly and then call device_unregister(),
> please reference drivers/base/*.c (e.g node.c or cpu.c).

Hmm... isn't this already taken care of by __sysfs_remove_dir() which
device_unregister() calls?  That function removes all non-directory
files under the directory being removed.

Thanks.

-- 
tejun

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Chen Gang]

On 05/16/2013 05:22 AM, Tejun Heo wrote:
> On Wed, May 15, 2013 at 02:13:14PM +0800, Chen Gang wrote:
>>
>> In workqueue_sysfs_register(), when failure occurs after called
>> device_create_file(), need call device_remove_file() to release the
>> related resources, then call device_unregister().
>>
>> Or it will cause issue.
>>
>> For individual 'device_attributs' (just like our case), need call
>> device_remove_file() explictly and then call device_unregister(),
>> please reference drivers/base/*.c (e.g node.c or cpu.c).
> 
> Hmm... isn't this already taken care of by __sysfs_remove_dir() which
> device_unregister() calls?  That function removes all non-directory
> files under the directory being removed.
> 

It just like what you said:
  device_unregister() ->
    kobject_del() ->
      sysfs_remove_dir() ->
        __sysfs_remove_dir() ->
          sysfs_remove_one()
But:
  device_remove_file() ->
    sysfs_remove_file() ->
      ...
      sysfs_attr_ns() ->
        ops->namespace()  (such as device_namespace() in workqueue.c)
      ...
      sysfs_hash_and_remove() ->
        sysfs_remove_one().

So if not call device_remove_file() explicitly, the device_namespace()
may be not called.


Even in device_unregister(), it still call device_remove_file() to
release the related attributes firstly, then call kobject_del().



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Chen Gang]

On 05/16/2013 11:33 AM, Chen Gang wrote:
> On 05/16/2013 05:22 AM, Tejun Heo wrote:
>> On Wed, May 15, 2013 at 02:13:14PM +0800, Chen Gang wrote:
>>>
>>> In workqueue_sysfs_register(), when failure occurs after called
>>> device_create_file(), need call device_remove_file() to release the
>>> related resources, then call device_unregister().
>>>
>>> Or it will cause issue.
>>>
>>> For individual 'device_attributs' (just like our case), need call
>>> device_remove_file() explictly and then call device_unregister(),
>>> please reference drivers/base/*.c (e.g node.c or cpu.c).
>>
>> Hmm... isn't this already taken care of by __sysfs_remove_dir() which
>> device_unregister() calls?  That function removes all non-directory
>> files under the directory being removed.
>>
> 
> It just like what you said:
>   device_unregister() ->
>     kobject_del() ->
>       sysfs_remove_dir() ->
>         __sysfs_remove_dir() ->
>           sysfs_remove_one()
> But:
>   device_remove_file() ->
>     sysfs_remove_file() ->
>       ...
>       sysfs_attr_ns() ->
>         ops->namespace()  (such as device_namespace() in workqueue.c)
>       ...
>       sysfs_hash_and_remove() ->
>         sysfs_remove_one().
> 
> So if not call device_remove_file() explicitly, the device_namespace()
> may be not called.
> 

It seems, if not call ops->namespace(), it still is OK. To get a result,
it still has much details to continue to read.

But all together, reference the related code of another subsystems, we
really need device_remove_file() before call device_unregister().


> 
> Even in device_unregister(), it still call device_remove_file() to
> release the related attributes firstly, then call kobject_del().
> 
> 
> 
> Thanks.
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Chen Gang]

On 05/16/2013 12:16 PM, Chen Gang wrote:
> On 05/16/2013 11:33 AM, Chen Gang wrote:
>> On 05/16/2013 05:22 AM, Tejun Heo wrote:
>>> On Wed, May 15, 2013 at 02:13:14PM +0800, Chen Gang wrote:
>>>>
>>>> In workqueue_sysfs_register(), when failure occurs after called
>>>> device_create_file(), need call device_remove_file() to release the
>>>> related resources, then call device_unregister().
>>>>
>>>> Or it will cause issue.
>>>>
>>>> For individual 'device_attributs' (just like our case), need call
>>>> device_remove_file() explictly and then call device_unregister(),
>>>> please reference drivers/base/*.c (e.g node.c or cpu.c).
>>>
>>> Hmm... isn't this already taken care of by __sysfs_remove_dir() which
>>> device_unregister() calls?  That function removes all non-directory
>>> files under the directory being removed.
>>>
>>

__sysfs_remove_dir() remove all related things, but not deref the count.

For __sysfs_remove_dir() ->
  ...
  sysfs_addrm_start()
  ...
  while() {
    ...
    sysfs_remove_one()  (not deref the related file)
  }
  sysfs_addrm_finish()  (will deref current dir)

For device_remove_file() ->
  sysfs_remove_file() ->
    sysfs_hash_and_remove() ->
      ...
      sysfs_addrm_start()
      ...
      sysfs_remove_one() (not deref the current file)
      sysfs_addrm_finish()  (will deref current file)


So if not call device_remove_file() firstly, the all files under the
directory are removed, but the related resources are not released which
will cause resource leak.


Thanks.

>> It just like what you said:
>>   device_unregister() ->
>>     kobject_del() ->
>>       sysfs_remove_dir() ->
>>         __sysfs_remove_dir() ->
>>           sysfs_remove_one()
>> But:
>>   device_remove_file() ->
>>     sysfs_remove_file() ->
>>       ...
>>       sysfs_attr_ns() ->
>>         ops->namespace()  (such as device_namespace() in workqueue.c)
>>       ...
>>       sysfs_hash_and_remove() ->
>>         sysfs_remove_one().
>>
>> So if not call device_remove_file() explicitly, the device_namespace()
>> may be not called.
>>
> 
> It seems, if not call ops->namespace(), it still is OK. To get a result,
> it still has much details to continue to read.
> 
> But all together, reference the related code of another subsystems, we
> really need device_remove_file() before call device_unregister().
> 
> 
>>
>> Even in device_unregister(), it still call device_remove_file() to
>> release the related attributes firstly, then call kobject_del().
>>
>>
>>
>> Thanks.
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Tejun Heo]

Hey,

On Thu, May 16, 2013 at 12:48:43PM +0800, Chen Gang wrote:
> __sysfs_remove_dir() remove all related things, but not deref the count.
> 
> For __sysfs_remove_dir() ->
>   ...
>   sysfs_addrm_start()
>   ...
>   while() {
>     ...
>     sysfs_remove_one()  (not deref the related file)
>   }
>   sysfs_addrm_finish()  (will deref current dir)
> 
> For device_remove_file() ->
>   sysfs_remove_file() ->
>     sysfs_hash_and_remove() ->
>       ...
>       sysfs_addrm_start()
>       ...
>       sysfs_remove_one() (not deref the current file)
>       sysfs_addrm_finish()  (will deref current file)
> 
> 
> So if not call device_remove_file() firstly, the all files under the
> directory are removed, but the related resources are not released which
> will cause resource leak.

Can you please be more specific on what gets leaked and if possible
fix it from sysfs side?

Thanks.

-- 
tejun

Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file()

[Chen Gang]

On 05/17/2013 01:50 AM, Tejun Heo wrote:
> Hey,
> 
> On Thu, May 16, 2013 at 12:48:43PM +0800, Chen Gang wrote:
>> __sysfs_remove_dir() remove all related things, but not deref the count.
>>
>> For __sysfs_remove_dir() ->
>>   ...
>>   sysfs_addrm_start()
>>   ...
>>   while() {
>>     ...
>>     sysfs_remove_one()  (not deref the related file)
>>   }
>>   sysfs_addrm_finish()  (will deref current dir)
>>
>> For device_remove_file() ->
>>   sysfs_remove_file() ->
>>     sysfs_hash_and_remove() ->
>>       ...
>>       sysfs_addrm_start()
>>       ...
>>       sysfs_remove_one() (not deref the current file)
>>       sysfs_addrm_finish()  (will deref current file)
>>
>>
>> So if not call device_remove_file() firstly, the all files under the
>> directory are removed, but the related resources are not released which
>> will cause resource leak.
> 
> Can you please be more specific on what gets leaked and if possible
> fix it from sysfs side?
> 

Oh, it is my fault. It is not the correct reason.

But I will continue to analyse why it should be add device_remove_file()
before device_unregister() in all 'drivers/*' and 'arch/*' source files.


Thanks.
-- 
Chen Gang

Asianux Corporation