2005-04-20 12:10:35

by Kei Tokunaga

[permalink] [raw]
Subject: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
---

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)


_


2005-04-20 12:35:19

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use


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


2005-04-20 17:35:23

by Greg KH

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-04-21 09:26:04

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-04-21 15:31:56

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
---

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)


_

2005-04-22 00:53:29

by Greg KH

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-04-22 02:32:53

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-04-25 14:04:24

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
---

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)


_

2005-04-26 06:54:49

by Greg KH

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-04-28 00:18:43

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-05-07 12:12:34

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
---

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)


_

2005-05-08 00:26:57

by Nathan Lynch

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

> 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 <[email protected]>
> ---
>
> 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

2005-05-08 12:29:44

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
---

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)


_

2005-05-09 22:44:16

by Matthew Dobson

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
> ---
>
> 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

2005-05-10 15:03:27

by Kei Tokunaga

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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 <[email protected]>
> > ---
> >
> > 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

2005-05-10 18:15:45

by Matthew Dobson

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-05-10 18:45:16

by Greg KH

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-05-10 18:58:45

by Matthew Dobson

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-05-10 20:11:50

by Greg KH

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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

2005-05-10 20:15:01

by Matthew Dobson

[permalink] [raw]
Subject: Re: [RFC/PATCH] unregister_node() for hotplug use

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