* [RFC/PATCH] unregister_node() for hotplug use @ 2005-04-20 12:07 Keiichiro Tokunaga 2005-04-20 12:35 ` Arjan van de Ven 2005-04-20 17:32 ` Greg KH 0 siblings, 2 replies; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-20 12:07 UTC (permalink / raw) To: linux-kernel, akpm; +Cc: Keiichiro Tokunaga This is to add a generic function 'unregister_node()'. It is used to remove objects of a node going away for hotplug. If CONFIG_HOTPLUG=y, it becomes available. This is against 2.6.12-rc2-mm3. Thanks, Keiichiro Tokunaga Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> --- linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 29 ++++++++++++++++++++++++-- linux-2.6.12-rc2-mm3-kei/include/linux/node.h | 6 ++++- 2 files changed, 32 insertions(+), 3 deletions(-) diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-14 20:49:37.000000000 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no * * Initialize and register the node device. */ -int __init register_node(struct node *node, int num, struct node *parent) +int __devinit register_node(struct node *node, int num, struct node *parent) { int error; @@ -145,6 +145,9 @@ int __init register_node(struct node *no error = sysdev_register(&node->sysdev); if (!error){ + /* + * If you add new object here, delete it when unregistering. + */ sysdev_create_file(&node->sysdev, &attr_cpumap); sysdev_create_file(&node->sysdev, &attr_meminfo); sysdev_create_file(&node->sysdev, &attr_numastat); @@ -153,8 +156,30 @@ int __init register_node(struct node *no return error; } +/* + * unregister_node - Remove objects of a node going away from sysfs. + * @node - node going away + * + * This is used only for hotplug. + */ +#ifdef CONFIG_HOTPLUG +void unregister_node(struct node *node) +{ + if (node == NULL) + return; + + sysdev_remove_file(&node->sysdev, &attr_cpumap); + sysdev_remove_file(&node->sysdev, &attr_meminfo); + sysdev_remove_file(&node->sysdev, &attr_numastat); + sysdev_remove_file(&node->sysdev, &attr_distance); + + sysdev_unregister(&node->sysdev); +} +EXPORT_SYMBOL(register_node); +EXPORT_SYMBOL(unregister_node); +#endif /* CONFIG_HOTPLUG */ -int __init register_node_type(void) +static int __init register_node_type(void) { return sysdev_class_register(&node_class); } diff -puN include/linux/node.h~numa_hp_base include/linux/node.h --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14 20:49:37.000000000 +0900 @@ -21,12 +21,16 @@ #include <linux/sysdev.h> #include <linux/cpumask.h> +#include <linux/module.h> struct node { struct sys_device sysdev; }; -extern int register_node(struct node *, int, struct node *); +extern int __devinit register_node(struct node *, int, struct node *); +#ifdef CONFIG_HOTPLUG +extern void unregister_node(struct node *node); +#endif #define to_node(sys_device) container_of(sys_device, struct node, sysdev) _ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-20 12:07 [RFC/PATCH] unregister_node() for hotplug use Keiichiro Tokunaga @ 2005-04-20 12:35 ` Arjan van de Ven 2005-04-21 9:25 ` Keiichiro Tokunaga 2005-04-20 17:32 ` Greg KH 1 sibling, 1 reply; 20+ messages in thread From: Arjan van de Ven @ 2005-04-20 12:35 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: linux-kernel, akpm > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-14 20:49:37.000000000 +0900 > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > +EXPORT_SYMBOL(register_node); > +EXPORT_SYMBOL(unregister_node); > +#endif /* CONFIG_HOTPLUG */ > please make these EXPORT_SYMBOL_GPL; the rest of sysfs is too and this is very much deep kernel internals that are linux specific. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-20 12:35 ` Arjan van de Ven @ 2005-04-21 9:25 ` Keiichiro Tokunaga 0 siblings, 0 replies; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-21 9:25 UTC (permalink / raw) To: Arjan van de Ven; +Cc: tokunaga.keiich, linux-kernel, akpm On Wed, 20 Apr 2005 14:35:10 +0200 Arjan van de Ven wrote: > > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-14 20:49:37.000000000 +0900 > > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > > > +EXPORT_SYMBOL(register_node); > > +EXPORT_SYMBOL(unregister_node); > > +#endif /* CONFIG_HOTPLUG */ > > > > please make these EXPORT_SYMBOL_GPL; the rest of sysfs is too and this > is very much deep kernel internals that are linux specific. OK, I will make that change. Thanks for the comments! Thanks, Keiichiro Tokunaga ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-20 12:07 [RFC/PATCH] unregister_node() for hotplug use Keiichiro Tokunaga 2005-04-20 12:35 ` Arjan van de Ven @ 2005-04-20 17:32 ` Greg KH 2005-04-21 15:30 ` Keiichiro Tokunaga 1 sibling, 1 reply; 20+ messages in thread From: Greg KH @ 2005-04-20 17:32 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: linux-kernel, akpm On Wed, Apr 20, 2005 at 09:07:44PM +0900, Keiichiro Tokunaga wrote: > This is to add a generic function 'unregister_node()'. > It is used to remove objects of a node going away for > hotplug. If CONFIG_HOTPLUG=y, it becomes available. > This is against 2.6.12-rc2-mm3. Please CC: this kind of stuff to the driver core maintainer, otherwise it can get dropped... Anyway, comments below: > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-14 20:49:37.000000000 +0900 > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > * > * Initialize and register the node device. > */ > -int __init register_node(struct node *node, int num, struct node *parent) > +int __devinit register_node(struct node *node, int num, struct node *parent) > { > int error; > > @@ -145,6 +145,9 @@ int __init register_node(struct node *no > error = sysdev_register(&node->sysdev); > > if (!error){ > + /* > + * If you add new object here, delete it when unregistering. > + */ Comment really isn't needed. > +/* > + * unregister_node - Remove objects of a node going away from sysfs. > + * @node - node going away > + * > + * This is used only for hotplug. > + */ If you are going to create function comments, at least use the proper kerneldoc format. > +#ifdef CONFIG_HOTPLUG You don't provide function prototype for when CONFIG_HOTPLUG is not enabled. > +void unregister_node(struct node *node) > +{ > + if (node == NULL) > + return; How can this happen? > + > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > + sysdev_remove_file(&node->sysdev, &attr_numastat); > + sysdev_remove_file(&node->sysdev, &attr_distance); > + > + sysdev_unregister(&node->sysdev); > +} > +EXPORT_SYMBOL(register_node); > +EXPORT_SYMBOL(unregister_node); All of sysfs and the driver core are EXPORT_SYMBOL_GPL(). Please follow that convention. > +#endif /* CONFIG_HOTPLUG */ > > -int __init register_node_type(void) > +static int __init register_node_type(void) Are you sure no one calls this? > { > return sysdev_class_register(&node_class); > } > diff -puN include/linux/node.h~numa_hp_base include/linux/node.h > --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14 20:49:37.000000000 +0900 > @@ -21,12 +21,16 @@ > > #include <linux/sysdev.h> > #include <linux/cpumask.h> > +#include <linux/module.h> Why? > > struct node { > struct sys_device sysdev; > }; > > -extern int register_node(struct node *, int, struct node *); > +extern int __devinit register_node(struct node *, int, struct node *); __devinit is not needed on a function prototype. > +#ifdef CONFIG_HOTPLUG > +extern void unregister_node(struct node *node); > +#endif Not needed for a function prototype. thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-20 17:32 ` Greg KH @ 2005-04-21 15:30 ` Keiichiro Tokunaga 2005-04-22 0:39 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-21 15:30 UTC (permalink / raw) To: Greg KH; +Cc: tokunaga.keiich, linux-kernel, akpm On Wed, 20 Apr 2005 10:32:35 -0700 Greg KH wrote: > On Wed, Apr 20, 2005 at 09:07:44PM +0900, Keiichiro Tokunaga wrote: > > This is to add a generic function 'unregister_node()'. > > It is used to remove objects of a node going away for > > hotplug. If CONFIG_HOTPLUG=y, it becomes available. > > This is against 2.6.12-rc2-mm3. > > Please CC: this kind of stuff to the driver core maintainer, otherwise > it can get dropped... I will for sure CC appropriate maintainers next time... > Anyway, comments below: > > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > > --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > > +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-14 20:49:37.000000000 +0900 > > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > * > > * Initialize and register the node device. > > */ > > -int __init register_node(struct node *node, int num, struct node *parent) > > +int __devinit register_node(struct node *node, int num, struct node *parent) > > { > > int error; > > > > @@ -145,6 +145,9 @@ int __init register_node(struct node *no > > error = sysdev_register(&node->sysdev); > > > > if (!error){ > > + /* > > + * If you add new object here, delete it when unregistering. > > + */ > > Comment really isn't needed. I removed the comments. > > +/* > > + * unregister_node - Remove objects of a node going away from sysfs. > > + * @node - node going away > > + * > > + * This is used only for hotplug. > > + */ > > If you are going to create function comments, at least use the proper > kerneldoc format. I removed it. I thought about its necessity again and it seems that the comments isn't really necessary because the purpose of the function is obvious anyway. > > +#ifdef CONFIG_HOTPLUG > > You don't provide function prototype for when CONFIG_HOTPLUG is not > enabled. > > > +void unregister_node(struct node *node) > > +{ > > + if (node == NULL) > > + return; > > How can this happen? It's a redundant check, so I removed it. > > + > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > + > > + sysdev_unregister(&node->sysdev); > > +} > > +EXPORT_SYMBOL(register_node); > > +EXPORT_SYMBOL(unregister_node); > > All of sysfs and the driver core are EXPORT_SYMBOL_GPL(). Please follow > that convention. OK, I made that change. > > +#endif /* CONFIG_HOTPLUG */ > > > > -int __init register_node_type(void) > > +static int __init register_node_type(void) > > Are you sure no one calls this? Yes, no one calls this today. > > { > > return sysdev_class_register(&node_class); > > } > > diff -puN include/linux/node.h~numa_hp_base include/linux/node.h > > --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 > > +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-14 20:49:37.000000000 +0900 > > @@ -21,12 +21,16 @@ > > > > #include <linux/sysdev.h> > > #include <linux/cpumask.h> > > +#include <linux/module.h> > > Why? > > > > > struct node { > > struct sys_device sysdev; > > }; > > > > -extern int register_node(struct node *, int, struct node *); > > +extern int __devinit register_node(struct node *, int, struct node *); > > __devinit is not needed on a function prototype. > > > +#ifdef CONFIG_HOTPLUG > > +extern void unregister_node(struct node *node); > > +#endif > > Not needed for a function prototype. I corrected them. Thanks for all the comments! I am attaching the updated patch. Please take a look. Thanks, Keiichiro Tokunaga Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> --- linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 21 +++++++++++++++++++-- linux-2.6.12-rc2-mm3-kei/include/linux/node.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-) diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-21 19:20:41.000000000 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no * * Initialize and register the node device. */ -int __init register_node(struct node *node, int num, struct node *parent) +int __devinit register_node(struct node *node, int num, struct node *parent) { int error; @@ -153,8 +153,25 @@ int __init register_node(struct node *no return error; } +#ifdef CONFIG_HOTPLUG +void unregister_node(struct node *node) +{ + sysdev_remove_file(&node->sysdev, &attr_cpumap); + sysdev_remove_file(&node->sysdev, &attr_meminfo); + sysdev_remove_file(&node->sysdev, &attr_numastat); + sysdev_remove_file(&node->sysdev, &attr_distance); + + sysdev_unregister(&node->sysdev); +} +EXPORT_SYMBOL_GPL(register_node); +EXPORT_SYMBOL_GPL(unregister_node); +#else /* !CONFIG_HOTPLUG */ +void unregister_node(struct node *node) +{ +} +#endif /* !CONFIG_HOTPLUG */ -int __init register_node_type(void) +static int __init register_node_type(void) { return sysdev_class_register(&node_class); } diff -puN include/linux/node.h~numa_hp_base include/linux/node.h --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-14 20:49:37.000000000 +0900 +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-21 13:48:58.000000000 +0900 @@ -27,6 +27,7 @@ struct node { }; extern int register_node(struct node *, int, struct node *); +extern void unregister_node(struct node *node); #define to_node(sys_device) container_of(sys_device, struct node, sysdev) _ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-21 15:30 ` Keiichiro Tokunaga @ 2005-04-22 0:39 ` Greg KH 2005-04-22 2:32 ` Keiichiro Tokunaga 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2005-04-22 0:39 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: Greg KH, linux-kernel, akpm On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > +#ifdef CONFIG_HOTPLUG > +void unregister_node(struct node *node) > +{ > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > + sysdev_remove_file(&node->sysdev, &attr_numastat); > + sysdev_remove_file(&node->sysdev, &attr_distance); > + > + sysdev_unregister(&node->sysdev); > +} > +EXPORT_SYMBOL_GPL(register_node); > +EXPORT_SYMBOL_GPL(unregister_node); > +#else /* !CONFIG_HOTPLUG */ > +void unregister_node(struct node *node) > +{ > +} > +#endif /* !CONFIG_HOTPLUG */ Oops, you only exported the symbols if CONFIG_HOTPLUG is enabled, not a good idea. Either make the null functions in a .h file if the option is not enabled, or always export them. And the comment for the #endif isn't really needed, as the whole #ifdef fits on a single screen :) And hey, what's the real big deal here, why not always have this function no matter if CONFIG_HOTPLUG is enabled or not? I really want to just make that an option that is always enabled anyway, but changable if you are using CONFIG_TINY or something... thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-22 0:39 ` Greg KH @ 2005-04-22 2:32 ` Keiichiro Tokunaga 2005-04-25 14:03 ` Keiichiro Tokunaga 0 siblings, 1 reply; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-22 2:32 UTC (permalink / raw) To: Greg KH; +Cc: tokunaga.keiich, linux-kernel, akpm On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: > On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > > +#ifdef CONFIG_HOTPLUG > > +void unregister_node(struct node *node) > > +{ > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > + > > + sysdev_unregister(&node->sysdev); > > +} > > +EXPORT_SYMBOL_GPL(register_node); > > +EXPORT_SYMBOL_GPL(unregister_node); > > +#else /* !CONFIG_HOTPLUG */ > > +void unregister_node(struct node *node) > > +{ > > +} > > +#endif /* !CONFIG_HOTPLUG */ > > Oops, you only exported the symbols if CONFIG_HOTPLUG is enabled, not a > good idea. Either make the null functions in a .h file if the option is > not enabled, or always export them. My bad... > And the comment for the #endif isn't really needed, as the whole #ifdef > fits on a single screen :) I see:) That makes sense. > And hey, what's the real big deal here, why not always have this > function no matter if CONFIG_HOTPLUG is enabled or not? I really want > to just make that an option that is always enabled anyway, but changable > if you are using CONFIG_TINY or something... I put the #ifdef there for users who don't need hotplug stuffs, but I want to make the option always enabled, too. Also a good side effect, the code would be cleaner:) I will be updating my patch without the #ifdef and sending it here. Thanks! Keiichiro Tokunaga ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-22 2:32 ` Keiichiro Tokunaga @ 2005-04-25 14:03 ` Keiichiro Tokunaga 2005-04-26 6:54 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-25 14:03 UTC (permalink / raw) To: gregkh; +Cc: tokunaga.keiich, linux-kernel, akpm On Fri, 22 Apr 2005 11:32:11 +0900 Keiichiro Tokunaga wrote: > On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: > > On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > > > +#ifdef CONFIG_HOTPLUG > > > +void unregister_node(struct node *node) > > > +{ > > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > > + > > > + sysdev_unregister(&node->sysdev); > > > +} > > > +EXPORT_SYMBOL_GPL(register_node); > > > +EXPORT_SYMBOL_GPL(unregister_node); > > > +#else /* !CONFIG_HOTPLUG */ > > > +void unregister_node(struct node *node) > > > +{ > > > +} > > > +#endif /* !CONFIG_HOTPLUG */ <snip> > > And hey, what's the real big deal here, why not always have this > > function no matter if CONFIG_HOTPLUG is enabled or not? I really want > > to just make that an option that is always enabled anyway, but changable > > if you are using CONFIG_TINY or something... > > I put the #ifdef there for users who don't need hotplug > stuffs, but I want to make the option always enabled, too. > Also a good side effect, the code would be cleaner:) I > will be updating my patch without the #ifdef and sending > it here. Here is the patch. Please apply. Thanks, Keiichiro Tokunaga Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> --- linux-2.6.12-rc2-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- linux-2.6.12-rc2-mm3-kei/include/linux/node.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-) diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc2-mm3/drivers/base/node.c~numa_hp_base 2005-04-25 22:37:53.064182320 +0900 +++ linux-2.6.12-rc2-mm3-kei/drivers/base/node.c 2005-04-25 22:41:02.744844059 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no * * Initialize and register the node device. */ -int __init register_node(struct node *node, int num, struct node *parent) +int __devinit register_node(struct node *node, int num, struct node *parent) { int error; @@ -153,8 +153,19 @@ int __init register_node(struct node *no return error; } +void unregister_node(struct node *node) +{ + sysdev_remove_file(&node->sysdev, &attr_cpumap); + sysdev_remove_file(&node->sysdev, &attr_meminfo); + sysdev_remove_file(&node->sysdev, &attr_numastat); + sysdev_remove_file(&node->sysdev, &attr_distance); + + sysdev_unregister(&node->sysdev); +} +EXPORT_SYMBOL_GPL(register_node); +EXPORT_SYMBOL_GPL(unregister_node); -int __init register_node_type(void) +static int __init register_node_type(void) { return sysdev_class_register(&node_class); } diff -puN include/linux/node.h~numa_hp_base include/linux/node.h --- linux-2.6.12-rc2-mm3/include/linux/node.h~numa_hp_base 2005-04-25 22:37:53.067112007 +0900 +++ linux-2.6.12-rc2-mm3-kei/include/linux/node.h 2005-04-25 22:37:53.069065132 +0900 @@ -27,6 +27,7 @@ struct node { }; extern int register_node(struct node *, int, struct node *); +extern void unregister_node(struct node *node); #define to_node(sys_device) container_of(sys_device, struct node, sysdev) _ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-25 14:03 ` Keiichiro Tokunaga @ 2005-04-26 6:54 ` Greg KH 2005-04-28 0:17 ` Keiichiro Tokunaga 2005-05-07 12:11 ` Keiichiro Tokunaga 0 siblings, 2 replies; 20+ messages in thread From: Greg KH @ 2005-04-26 6:54 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: linux-kernel, akpm On Mon, Apr 25, 2005 at 11:03:33PM +0900, Keiichiro Tokunaga wrote: > On Fri, 22 Apr 2005 11:32:11 +0900 Keiichiro Tokunaga wrote: > > On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: > > > On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > > > > +#ifdef CONFIG_HOTPLUG > > > > +void unregister_node(struct node *node) > > > > +{ > > > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > > > + > > > > + sysdev_unregister(&node->sysdev); > > > > +} > > > > +EXPORT_SYMBOL_GPL(register_node); > > > > +EXPORT_SYMBOL_GPL(unregister_node); > > > > +#else /* !CONFIG_HOTPLUG */ > > > > +void unregister_node(struct node *node) > > > > +{ > > > > +} > > > > +#endif /* !CONFIG_HOTPLUG */ > <snip> > > > And hey, what's the real big deal here, why not always have this > > > function no matter if CONFIG_HOTPLUG is enabled or not? I really want > > > to just make that an option that is always enabled anyway, but changable > > > if you are using CONFIG_TINY or something... > > > > I put the #ifdef there for users who don't need hotplug > > stuffs, but I want to make the option always enabled, too. > > Also a good side effect, the code would be cleaner:) I > > will be updating my patch without the #ifdef and sending > > it here. > > Here is the patch. Please apply. Care to resend it with a proper change log description that I can use? thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-26 6:54 ` Greg KH @ 2005-04-28 0:17 ` Keiichiro Tokunaga 2005-05-07 12:11 ` Keiichiro Tokunaga 1 sibling, 0 replies; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-04-28 0:17 UTC (permalink / raw) To: Greg KH; +Cc: tokunaga.keiich, linux-kernel, akpm On Mon, 25 Apr 2005 23:54:32 -0700 Greg KH wrote: > On Mon, Apr 25, 2005 at 11:03:33PM +0900, Keiichiro Tokunaga wrote: > > On Fri, 22 Apr 2005 11:32:11 +0900 Keiichiro Tokunaga wrote: > > > On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: > > > > On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > > > > > +#ifdef CONFIG_HOTPLUG > > > > > +void unregister_node(struct node *node) > > > > > +{ > > > > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > > > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > > > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > > > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > > > > + > > > > > + sysdev_unregister(&node->sysdev); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(register_node); > > > > > +EXPORT_SYMBOL_GPL(unregister_node); > > > > > +#else /* !CONFIG_HOTPLUG */ > > > > > +void unregister_node(struct node *node) > > > > > +{ > > > > > +} > > > > > +#endif /* !CONFIG_HOTPLUG */ > > <snip> > > > > And hey, what's the real big deal here, why not always have this > > > > function no matter if CONFIG_HOTPLUG is enabled or not? I really want > > > > to just make that an option that is always enabled anyway, but changable > > > > if you are using CONFIG_TINY or something... > > > > > > I put the #ifdef there for users who don't need hotplug > > > stuffs, but I want to make the option always enabled, too. > > > Also a good side effect, the code would be cleaner:) I > > > will be updating my patch without the #ifdef and sending > > > it here. > > > > Here is the patch. Please apply. > > Care to resend it with a proper change log description that I can use? Sure. But, please let me ask you something before I post the update patch. I think register_node() also should be always there if unregister_node() is always there. So, the '__devinit' attribute for register_node() does not seem to be necessary. (Actually, the attribute '__init' of register_node() was replaced with '__devinit' in my previous patch.) What do you think of this? Thanks, Keiichiro Tokunaga ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-04-26 6:54 ` Greg KH 2005-04-28 0:17 ` Keiichiro Tokunaga @ 2005-05-07 12:11 ` Keiichiro Tokunaga 2005-05-08 0:26 ` Nathan Lynch 2005-05-09 22:44 ` Matthew Dobson 1 sibling, 2 replies; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-05-07 12:11 UTC (permalink / raw) To: Greg KH; +Cc: tokunaga.keiich, linux-kernel, akpm On Mon, 25 Apr 2005 23:54:32 -0700 Greg KH wrote: > On Mon, Apr 25, 2005 at 11:03:33PM +0900, Keiichiro Tokunaga wrote: > > On Fri, 22 Apr 2005 11:32:11 +0900 Keiichiro Tokunaga wrote: > > > On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: > > > > On Fri, Apr 22, 2005 at 12:30:09AM +0900, Keiichiro Tokunaga wrote: > > > > > +#ifdef CONFIG_HOTPLUG > > > > > +void unregister_node(struct node *node) > > > > > +{ > > > > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > > > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > > > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > > > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > > > > + > > > > > + sysdev_unregister(&node->sysdev); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(register_node); > > > > > +EXPORT_SYMBOL_GPL(unregister_node); > > > > > +#else /* !CONFIG_HOTPLUG */ > > > > > +void unregister_node(struct node *node) > > > > > +{ > > > > > +} > > > > > +#endif /* !CONFIG_HOTPLUG */ > > <snip> > > > > And hey, what's the real big deal here, why not always have this > > > > function no matter if CONFIG_HOTPLUG is enabled or not? I really want > > > > to just make that an option that is always enabled anyway, but changable > > > > if you are using CONFIG_TINY or something... > > > > > > I put the #ifdef there for users who don't need hotplug > > > stuffs, but I want to make the option always enabled, too. > > > Also a good side effect, the code would be cleaner:) I > > > will be updating my patch without the #ifdef and sending > > > it here. > > > > Here is the patch. Please apply. > > Care to resend it with a proper change log description that I can use? I updated the patch for 2.6.12-rc3-mm3 based on my previous comments. Please apply. Thanks, Keiichiro Tokunaga This adds a generic function 'unregister_node()'. It is used to remove objects of a node going away for hotplug. Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> --- linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-) diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-07 19:58:15.000000000 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no * * Initialize and register the node device. */ -int __init register_node(struct node *node, int num, struct node *parent) +int register_node(struct node *node, int num, struct node *parent) { int error; @@ -153,8 +153,19 @@ int __init register_node(struct node *no return error; } +void unregister_node(struct node *node) +{ + sysdev_remove_file(&node->sysdev, &attr_cpumap); + sysdev_remove_file(&node->sysdev, &attr_meminfo); + sysdev_remove_file(&node->sysdev, &attr_numastat); + sysdev_remove_file(&node->sysdev, &attr_distance); + + sysdev_unregister(&node->sysdev); +} +EXPORT_SYMBOL_GPL(register_node); +EXPORT_SYMBOL_GPL(unregister_node); -int __init register_node_type(void) +static int __init register_node_type(void) { return sysdev_class_register(&node_class); } diff -puN include/linux/node.h~numa_hp_base include/linux/node.h --- linux-2.6.12-rc3-mm3/include/linux/node.h~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 +++ linux-2.6.12-rc3-mm3-kei/include/linux/node.h 2005-05-07 19:58:15.000000000 +0900 @@ -27,6 +27,7 @@ struct node { }; extern int register_node(struct node *, int, struct node *); +extern void unregister_node(struct node *node); #define to_node(sys_device) container_of(sys_device, struct node, sysdev) _ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-07 12:11 ` Keiichiro Tokunaga @ 2005-05-08 0:26 ` Nathan Lynch 2005-05-08 12:28 ` Keiichiro Tokunaga 2005-05-09 22:44 ` Matthew Dobson 1 sibling, 1 reply; 20+ messages in thread From: Nathan Lynch @ 2005-05-08 0:26 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: Greg KH, linux-kernel, akpm > This adds a generic function 'unregister_node()'. > It is used to remove objects of a node going away > for hotplug. > > Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> > --- > > linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- > linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 > +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-07 19:58:15.000000000 +0900 > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > * > * Initialize and register the node device. > */ > -int __init register_node(struct node *node, int num, struct node *parent) > +int register_node(struct node *node, int num, struct node *parent) > { > int error; > > @@ -153,8 +153,19 @@ int __init register_node(struct node *no > return error; > } > > +void unregister_node(struct node *node) > +{ > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > + sysdev_remove_file(&node->sysdev, &attr_numastat); > + sysdev_remove_file(&node->sysdev, &attr_distance); > + > + sysdev_unregister(&node->sysdev); > +} Is it a bug to call unregister_node() if there are still cpus or memory present in the node? Note that register_cpu() creates a symlink under the node directory to the cpu -- are you assuming that all the node's cpu sysdevs will have been unregistered by the time unregister_node is called? If so, is it possible to enforce that, or at least document it? > +EXPORT_SYMBOL_GPL(register_node); > +EXPORT_SYMBOL_GPL(unregister_node); What module code needs to call these? ACPI? Nathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-08 0:26 ` Nathan Lynch @ 2005-05-08 12:28 ` Keiichiro Tokunaga 0 siblings, 0 replies; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-05-08 12:28 UTC (permalink / raw) To: Nathan Lynch, gregkh; +Cc: tokunaga.keiich, linux-kernel, akpm On Sat, 07 May 2005 19:26:48 -0500 Nathan Lynch wrote: > > This adds a generic function 'unregister_node()'. > > It is used to remove objects of a node going away > > for hotplug. > > > > Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> > > --- > > > > linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- > > linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > > --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 > > +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-07 19:58:15.000000000 +0900 > > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > * > > * Initialize and register the node device. > > */ > > -int __init register_node(struct node *node, int num, struct node *parent) > > +int register_node(struct node *node, int num, struct node *parent) > > { > > int error; > > > > @@ -153,8 +153,19 @@ int __init register_node(struct node *no > > return error; > > } > > > > +void unregister_node(struct node *node) > > +{ > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > + > > + sysdev_unregister(&node->sysdev); > > +} > > Is it a bug to call unregister_node() if there are still cpus or > memory present in the node? Note that register_cpu() creates a > symlink under the node directory to the cpu -- are you assuming that > all the node's cpu sysdevs will have been unregistered by the time > unregister_node is called? If so, is it possible to enforce that, or > at least document it? Yes, this code is assuming that. I will document it in a function description of unregister_node(). > > +EXPORT_SYMBOL_GPL(register_node); > > +EXPORT_SYMBOL_GPL(unregister_node); > > What module code needs to call these? ACPI? Nobody today. I thought it was necessary for my another patch to support node hotplug, but I found out it's not. My mistake... I will remove them. I'm attaching an updated patch. Thanks, Keiichiro Tokunaga This adds a generic function 'unregister_node()'. It is used to remove objects of a node going away for hotplug. All the devices on the node must be unregistered before calling this function. Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> --- linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 20 ++++++++++++++++++-- linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-08 20:27:32.000000000 +0900 @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no * * Initialize and register the node device. */ -int __init register_node(struct node *node, int num, struct node *parent) +int register_node(struct node *node, int num, struct node *parent) { int error; @@ -153,8 +153,24 @@ int __init register_node(struct node *no return error; } +/** + * unregister_node - unregister a node device + * @node: node going away + * + * Unregisters a node device @node. All the devices on the node must be + * unregistered before calling this function. + */ +void unregister_node(struct node *node) +{ + sysdev_remove_file(&node->sysdev, &attr_cpumap); + sysdev_remove_file(&node->sysdev, &attr_meminfo); + sysdev_remove_file(&node->sysdev, &attr_numastat); + sysdev_remove_file(&node->sysdev, &attr_distance); + + sysdev_unregister(&node->sysdev); +} -int __init register_node_type(void) +static int __init register_node_type(void) { return sysdev_class_register(&node_class); } diff -puN include/linux/node.h~numa_hp_base include/linux/node.h --- linux-2.6.12-rc3-mm3/include/linux/node.h~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 +++ linux-2.6.12-rc3-mm3-kei/include/linux/node.h 2005-05-07 19:58:15.000000000 +0900 @@ -27,6 +27,7 @@ struct node { }; extern int register_node(struct node *, int, struct node *); +extern void unregister_node(struct node *node); #define to_node(sys_device) container_of(sys_device, struct node, sysdev) _ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-07 12:11 ` Keiichiro Tokunaga 2005-05-08 0:26 ` Nathan Lynch @ 2005-05-09 22:44 ` Matthew Dobson 2005-05-10 11:20 ` Keiichiro Tokunaga 1 sibling, 1 reply; 20+ messages in thread From: Matthew Dobson @ 2005-05-09 22:44 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: Greg KH, linux-kernel, akpm Keiichiro Tokunaga wrote: > I updated the patch for 2.6.12-rc3-mm3 based on my > previous comments. Please apply. > > Thanks, > Keiichiro Tokunaga > > > > This adds a generic function 'unregister_node()'. > It is used to remove objects of a node going away > for hotplug. > > Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> > --- > > linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- > linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 > +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-07 19:58:15.000000000 +0900 > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > * > * Initialize and register the node device. > */ > -int __init register_node(struct node *node, int num, struct node *parent) > +int register_node(struct node *node, int num, struct node *parent) > { > int error; > > @@ -153,8 +153,19 @@ int __init register_node(struct node *no > return error; > } > > +void unregister_node(struct node *node) > +{ > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > + sysdev_remove_file(&node->sysdev, &attr_numastat); > + sysdev_remove_file(&node->sysdev, &attr_distance); > + > + sysdev_unregister(&node->sysdev); > +} Is there a reason to not make both register_node() and unregister_node() __devinit? If a user has CONFIG_HOTPLUG=y then they want these functions, otherwise there is no point, as they promised they won't be hotplugging anything, right? -Matt ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-09 22:44 ` Matthew Dobson @ 2005-05-10 11:20 ` Keiichiro Tokunaga 2005-05-10 18:15 ` Matthew Dobson 0 siblings, 1 reply; 20+ messages in thread From: Keiichiro Tokunaga @ 2005-05-10 11:20 UTC (permalink / raw) To: Matthew Dobson; +Cc: tokunaga.keiich, gregkh, linux-kernel, akpm On Mon, 09 May 2005 15:44:03 -0700 Matthew Dobson wrote: > Keiichiro Tokunaga wrote: > > I updated the patch for 2.6.12-rc3-mm3 based on my > > previous comments. Please apply. > > > > Thanks, > > Keiichiro Tokunaga > > > > > > > > This adds a generic function 'unregister_node()'. > > It is used to remove objects of a node going away > > for hotplug. > > > > Signed-off-by: Keiichiro Tokunaga <tokunaga.keiich@jp.fujitsu.com> > > --- > > > > linux-2.6.12-rc3-mm3-kei/drivers/base/node.c | 15 +++++++++++++-- > > linux-2.6.12-rc3-mm3-kei/include/linux/node.h | 1 + > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff -puN drivers/base/node.c~numa_hp_base drivers/base/node.c > > --- linux-2.6.12-rc3-mm3/drivers/base/node.c~numa_hp_base 2005-05-07 19:58:15.000000000 +0900 > > +++ linux-2.6.12-rc3-mm3-kei/drivers/base/node.c 2005-05-07 19:58:15.000000000 +0900 > > @@ -136,7 +136,7 @@ static SYSDEV_ATTR(distance, S_IRUGO, no > > * > > * Initialize and register the node device. > > */ > > -int __init register_node(struct node *node, int num, struct node *parent) > > +int register_node(struct node *node, int num, struct node *parent) > > { > > int error; > > > > @@ -153,8 +153,19 @@ int __init register_node(struct node *no > > return error; > > } > > > > +void unregister_node(struct node *node) > > +{ > > + sysdev_remove_file(&node->sysdev, &attr_cpumap); > > + sysdev_remove_file(&node->sysdev, &attr_meminfo); > > + sysdev_remove_file(&node->sysdev, &attr_numastat); > > + sysdev_remove_file(&node->sysdev, &attr_distance); > > + > > + sysdev_unregister(&node->sysdev); > > +} > > Is there a reason to not make both register_node() and unregister_node() > __devinit? If a user has CONFIG_HOTPLUG=y then they want these functions, > otherwise there is no point, as they promised they won't be hotplugging > anything, right? The main reason is Greg advised me that we had the function no matter if CONFIG_HOTPLUG is true or not. An addtional advantage of this is that the source becomes cleaner because I included unregister_node() with '#ifdef CONFIG_HOTPLUG' and '#endif' in my previous version of patch. Thanks, Keiichiro Tokunaga ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-10 11:20 ` Keiichiro Tokunaga @ 2005-05-10 18:15 ` Matthew Dobson 2005-05-10 18:45 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Matthew Dobson @ 2005-05-10 18:15 UTC (permalink / raw) To: Keiichiro Tokunaga; +Cc: gregkh, linux-kernel, akpm Keiichiro Tokunaga wrote: > On Mon, 09 May 2005 15:44:03 -0700 Matthew Dobson wrote: >>Is there a reason to not make both register_node() and unregister_node() >>__devinit? If a user has CONFIG_HOTPLUG=y then they want these functions, >>otherwise there is no point, as they promised they won't be hotplugging >>anything, right? > > The main reason is Greg advised me that we had the function > no matter if CONFIG_HOTPLUG is true or not. An addtional > advantage of this is that the source becomes cleaner because > I included unregister_node() with '#ifdef CONFIG_HOTPLUG' and > '#endif' in my previous version of patch. > > Thanks, > Keiichiro Tokunaga > You're referring to this comment: On Thu, 21 Apr 2005 17:39:20 -0700 Greg KH wrote: >> And hey, what's the real big deal here, why not always have this >> function no matter if CONFIG_HOTPLUG is enabled or not? I really want >> to just make that an option that is always enabled anyway, but changable >> if you are using CONFIG_TINY or something... correct? I think GregKH was referring to you #ifdef'ing the function away so that it isn't even there if CONFIG_HOTPLUG=n, which is completely different from marking the function as __devinit. If you mark the function as __devinit, it will still be there for the kernel initialization phase whether CONFIG_HOTPLUG is on or off. What it does mean, however, is that the function will get freed, along with the rest of the functions/data marked __init or __initdata, if CONFIG_HOTPLUG is off. If CONFIG_HOTPLUG is on, __devinit is defined to be nothing and the function will remain because there is a possibility that someone will call the function after the initialization phase. If CONFIG_HOTPLUG is off, the user is assuring us that no devices will be hot-added or hot-removed, so there is no point in bloating the kernel text (albeit very slightly) with functions that we *know* won't be called. So I think it's probably a good idea to stick the __devinit on register_node() and unregister_node(), otherwise we have no marker to know which functions to remove for CONFIG_TINY. Greg? -Matt ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-10 18:15 ` Matthew Dobson @ 2005-05-10 18:45 ` Greg KH 2005-05-10 18:58 ` Matthew Dobson 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2005-05-10 18:45 UTC (permalink / raw) To: Matthew Dobson; +Cc: Keiichiro Tokunaga, linux-kernel, akpm On Tue, May 10, 2005 at 11:15:29AM -0700, Matthew Dobson wrote: > > So I think it's probably a good idea to stick the __devinit on > register_node() and unregister_node(), otherwise we have no marker to know > which functions to remove for CONFIG_TINY. Greg? Like _anyone_ would have CONFIG_NUMA and CONFIG_TINY enabled at the same time? I don't think so... I'll leave it as is for now. thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-10 18:45 ` Greg KH @ 2005-05-10 18:58 ` Matthew Dobson 2005-05-10 20:11 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Matthew Dobson @ 2005-05-10 18:58 UTC (permalink / raw) To: Greg KH; +Cc: Keiichiro Tokunaga, linux-kernel, akpm Greg KH wrote: > On Tue, May 10, 2005 at 11:15:29AM -0700, Matthew Dobson wrote: > >>So I think it's probably a good idea to stick the __devinit on >>register_node() and unregister_node(), otherwise we have no marker to know >>which functions to remove for CONFIG_TINY. Greg? > > > Like _anyone_ would have CONFIG_NUMA and CONFIG_TINY enabled at the same > time? I don't think so... > > I'll leave it as is for now. > > thanks, > > greg k-h No, it seems unlikely that anyone would build with CONFIG_NUMA and CONFIG_TINY both enabled. But it is possible and reasonable to build with CONFIG_NUMA=y and CONFIG_HOTPLUG=n, which is the case I was trying to speak to. If NUMA is on and HOTPLUG is off, then we're wasting kernel text (granted, it's a very small amount of space) for the register_node() & unregister_node() functions that we *know* will never be called after initial bootup. That's why I suggested marking both of those functions as __devinit. But it doesn't make a huge difference either way. -Matt ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-10 18:58 ` Matthew Dobson @ 2005-05-10 20:11 ` Greg KH 2005-05-10 20:13 ` Matthew Dobson 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2005-05-10 20:11 UTC (permalink / raw) To: Matthew Dobson; +Cc: Keiichiro Tokunaga, linux-kernel, akpm On Tue, May 10, 2005 at 11:58:36AM -0700, Matthew Dobson wrote: > Greg KH wrote: > > On Tue, May 10, 2005 at 11:15:29AM -0700, Matthew Dobson wrote: > > > >>So I think it's probably a good idea to stick the __devinit on > >>register_node() and unregister_node(), otherwise we have no marker to know > >>which functions to remove for CONFIG_TINY. Greg? > > > > > > Like _anyone_ would have CONFIG_NUMA and CONFIG_TINY enabled at the same > > time? I don't think so... > > > > I'll leave it as is for now. > > No, it seems unlikely that anyone would build with CONFIG_NUMA and > CONFIG_TINY both enabled. But it is possible and reasonable to build with > CONFIG_NUMA=y and CONFIG_HOTPLUG=n, which is the case I was trying to speak > to. If NUMA is on and HOTPLUG is off, then we're wasting kernel text > (granted, it's a very small amount of space) for the register_node() & > unregister_node() functions that we *know* will never be called after > initial bootup. That's why I suggested marking both of those functions as > __devinit. But it doesn't make a huge difference either way. I do not think this is an issue, and I want to move CONFIG_HOTPLUG to be under CONFIG_TINY anyway, so you could only disable it if TINY is enabled. But that's a different email thread... thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC/PATCH] unregister_node() for hotplug use 2005-05-10 20:11 ` Greg KH @ 2005-05-10 20:13 ` Matthew Dobson 0 siblings, 0 replies; 20+ messages in thread From: Matthew Dobson @ 2005-05-10 20:13 UTC (permalink / raw) To: Greg KH; +Cc: Keiichiro Tokunaga, linux-kernel, akpm Greg KH wrote: > On Tue, May 10, 2005 at 11:58:36AM -0700, Matthew Dobson wrote: > >>Greg KH wrote: >> >>>On Tue, May 10, 2005 at 11:15:29AM -0700, Matthew Dobson wrote: >>> >>> >>>>So I think it's probably a good idea to stick the __devinit on >>>>register_node() and unregister_node(), otherwise we have no marker to know >>>>which functions to remove for CONFIG_TINY. Greg? >>> >>> >>>Like _anyone_ would have CONFIG_NUMA and CONFIG_TINY enabled at the same >>>time? I don't think so... >>> >>>I'll leave it as is for now. >> >>No, it seems unlikely that anyone would build with CONFIG_NUMA and >>CONFIG_TINY both enabled. But it is possible and reasonable to build with >>CONFIG_NUMA=y and CONFIG_HOTPLUG=n, which is the case I was trying to speak >>to. If NUMA is on and HOTPLUG is off, then we're wasting kernel text >>(granted, it's a very small amount of space) for the register_node() & >>unregister_node() functions that we *know* will never be called after >>initial bootup. That's why I suggested marking both of those functions as >>__devinit. But it doesn't make a huge difference either way. > > > I do not think this is an issue, and I want to move CONFIG_HOTPLUG to be > under CONFIG_TINY anyway, so you could only disable it if TINY is > enabled. But that's a different email thread... > > thanks, > > greg k-h Fair enough. We'll leave those functions alone... -Matt ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2005-05-10 20:15 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-04-20 12:07 [RFC/PATCH] unregister_node() for hotplug use Keiichiro Tokunaga 2005-04-20 12:35 ` Arjan van de Ven 2005-04-21 9:25 ` Keiichiro Tokunaga 2005-04-20 17:32 ` Greg KH 2005-04-21 15:30 ` Keiichiro Tokunaga 2005-04-22 0:39 ` Greg KH 2005-04-22 2:32 ` Keiichiro Tokunaga 2005-04-25 14:03 ` Keiichiro Tokunaga 2005-04-26 6:54 ` Greg KH 2005-04-28 0:17 ` Keiichiro Tokunaga 2005-05-07 12:11 ` Keiichiro Tokunaga 2005-05-08 0:26 ` Nathan Lynch 2005-05-08 12:28 ` Keiichiro Tokunaga 2005-05-09 22:44 ` Matthew Dobson 2005-05-10 11:20 ` Keiichiro Tokunaga 2005-05-10 18:15 ` Matthew Dobson 2005-05-10 18:45 ` Greg KH 2005-05-10 18:58 ` Matthew Dobson 2005-05-10 20:11 ` Greg KH 2005-05-10 20:13 ` Matthew Dobson
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).