* [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init() @ 2013-05-14 12:25 Chen Gang 2013-05-14 15:17 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-14 12:25 UTC (permalink / raw) To: tj; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init() 2013-05-14 12:25 [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init() Chen Gang @ 2013-05-14 15:17 ` Tejun Heo 2013-05-15 2:16 ` Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2013-05-14 15:17 UTC (permalink / raw) To: Chen Gang; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init() 2013-05-14 15:17 ` Tejun Heo @ 2013-05-15 2:16 ` Chen Gang 2013-05-15 6:13 ` [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-15 2:16 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-15 2:16 ` Chen Gang @ 2013-05-15 6:13 ` Chen Gang 2013-05-15 21:22 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-15 6:13 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-15 6:13 ` [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() Chen Gang @ 2013-05-15 21:22 ` Tejun Heo 2013-05-16 3:33 ` Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2013-05-15 21:22 UTC (permalink / raw) To: Chen Gang; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-15 21:22 ` Tejun Heo @ 2013-05-16 3:33 ` Chen Gang 2013-05-16 4:16 ` Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-16 3:33 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-16 3:33 ` Chen Gang @ 2013-05-16 4:16 ` Chen Gang 2013-05-16 4:48 ` Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-16 4:16 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-16 4:16 ` Chen Gang @ 2013-05-16 4:48 ` Chen Gang 2013-05-16 17:50 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-05-16 4:48 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-16 4:48 ` Chen Gang @ 2013-05-16 17:50 ` Tejun Heo 2013-05-17 2:51 ` Chen Gang 0 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2013-05-16 17:50 UTC (permalink / raw) To: Chen Gang; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() 2013-05-16 17:50 ` Tejun Heo @ 2013-05-17 2:51 ` Chen Gang 0 siblings, 0 replies; 10+ messages in thread From: Chen Gang @ 2013-05-17 2:51 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-05-17 2:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-14 12:25 [PATCH] kernel/workqueue.c: better to free related resources when failure occurs in wq_numa_init() Chen Gang 2013-05-14 15:17 ` Tejun Heo 2013-05-15 2:16 ` Chen Gang 2013-05-15 6:13 ` [PATCH] kernel/workqueue.c: need call device_remove_file() when failure occurs after called device_create_file() Chen Gang 2013-05-15 21:22 ` Tejun Heo 2013-05-16 3:33 ` Chen Gang 2013-05-16 4:16 ` Chen Gang 2013-05-16 4:48 ` Chen Gang 2013-05-16 17:50 ` Tejun Heo 2013-05-17 2:51 ` Chen Gang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox