2006-08-10 19:39:27

by Andi Kleen

Subject: [PATCH for review] [130/145] i386: clean up topology.c

r

From: Magnus Damm <[email protected]>

There is no need to duplicate the topology_init() function.

Signed-off-by: Magnus Damm <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>

---

arch/i386/kernel/topology.c | 21 +++------------------
1 files changed, 3 insertions(+), 18 deletions(-)

Index: linux/arch/i386/kernel/topology.c
===================================================================
--- linux.orig/arch/i386/kernel/topology.c
+++ linux/arch/i386/kernel/topology.c
@@ -28,6 +28,7 @@
#include <linux/init.h>
#include <linux/smp.h>
#include <linux/nodemask.h>
+#include <linux/mmzone.h>
#include <asm/cpu.h>

static struct i386_cpu cpu_devices[NR_CPUS];
@@ -55,34 +56,18 @@ EXPORT_SYMBOL(arch_register_cpu);
EXPORT_SYMBOL(arch_unregister_cpu);
#endif /*CONFIG_HOTPLUG_CPU*/

-
-
-#ifdef CONFIG_NUMA
-#include <linux/mmzone.h>
-
static int __init topology_init(void)
{
int i;

+#ifdef CONFIG_NUMA
for_each_online_node(i)
register_one_node(i);
+#endif /* CONFIG_NUMA */

for_each_present_cpu(i)
arch_register_cpu(i);
return 0;
}

-#else /* !CONFIG_NUMA */
-
-static int __init topology_init(void)
-{
- int i;
-
- for_each_present_cpu(i)
- arch_register_cpu(i);
- return 0;
-}
-
-#endif /* CONFIG_NUMA */
-
subsys_initcall(topology_init);

2006-08-10 19:50:30

by Dave Hansen

Subject: Re: [PATCH for review] [130/145] i386: clean up topology.c

On Thu, 2006-08-10 at 21:37 +0200, Andi Kleen wrote:
> static int __init topology_init(void)
> {
> int i;
>
> +#ifdef CONFIG_NUMA
> for_each_online_node(i)
> register_one_node(i);
> +#endif /* CONFIG_NUMA */
>
> for_each_present_cpu(i)
> arch_register_cpu(i);
> return 0;
> }

Wouldn't it be more proper here to make register_one_node() have a
non-NUMA definition, instead of putting an #ifdef in a .c file like
this?

-- Dave

2006-08-10 19:56:05

by Andi Kleen

Subject: Re: [PATCH for review] [130/145] i386: clean up topology.c

On Thursday 10 August 2006 21:50, Dave Hansen wrote:
> On Thu, 2006-08-10 at 21:37 +0200, Andi Kleen wrote:
> > static int __init topology_init(void)
> > {
> > int i;
> >
> > +#ifdef CONFIG_NUMA
> > for_each_online_node(i)
> > register_one_node(i);
> > +#endif /* CONFIG_NUMA */
> >
> > for_each_present_cpu(i)
> > arch_register_cpu(i);
> > return 0;
> > }
>
> Wouldn't it be more proper here to make register_one_node() have a
> non-NUMA definition, instead of putting an #ifdef in a .c file like
> this?

I don't see a particular advantage of that for something simple like this.
But if you feel strongly about it please submit a tested replacement patch.

-Andi

2006-08-11 01:32:27

by Magnus Damm

Subject: Re: [PATCH for review] [130/145] i386: clean up topology.c

On 8/11/06, Dave Hansen <[email protected]> wrote:
> On Thu, 2006-08-10 at 21:37 +0200, Andi Kleen wrote:
> > static int __init topology_init(void)
> > {
> > int i;
> >
> > +#ifdef CONFIG_NUMA
> > for_each_online_node(i)
> > register_one_node(i);
> > +#endif /* CONFIG_NUMA */
> >
> > for_each_present_cpu(i)
> > arch_register_cpu(i);
> > return 0;
> > }
>
> Wouldn't it be more proper here to make register_one_node() have a
> non-NUMA definition, instead of putting an #ifdef in a .c file like
> this?

I thought about that too, and my reason for not doing it is that this
simple fix would be less straight-forward and probably more subject to
whining and arguing. So my plan was to do this as a first step and
then encourage anyone else that wanted to fix up register_one_node()
properly. =)

Cheers,

/ magnus