Re: [Xen-devel] [RFC PATCH v1 02/21] x86: NUMA: Refactor NUMA code

[Julien Grall <julien.grall@xxxxxxx>]

Hello Vijay,

On 22/02/17 10:04, Vijay Kilari wrote:
> On Mon, Feb 20, 2017 at 6:07 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > >  {
> > >      int rr, i;
> > > @@ -288,16 +145,6 @@ void __init numa_initmem_init(unsigned long
> > > start_pfn, unsigned long end_pfn)
> > >                      (u64)end_pfn << PAGE_SHIFT);
> > >  }
> > >
> > > -void numa_add_cpu(int cpu)
> > > -{
> > > -    cpumask_set_cpu(cpu, &node_to_cpumask[cpu_to_node(cpu)]);
> > > -}
> > > -
> > > -void numa_set_node(int cpu, nodeid_t node)
> > > -{
> > > -    cpu_to_node[cpu] = node;
> > > -}
> > > -
> > >  /* [numa=off] */
> > >  static __init int numa_setup(char *opt)
> >
> > Same question here. Everything in numa_setup and the fake NUMA looks
> > valid for ARM too.
> numa_setup() is taken in separate patch. fake numa case is not considered.
> for x86 it is under separate config CONFIG_NUMA_EMU.
Why fake numa is not considered? This would help to test the series on 
non-NUMA platform.
> > [....]
> >
> > > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > > index 0fed30b..c1bd2ff 100644
> > > --- a/xen/common/Makefile
> > > +++ b/xen/common/Makefile
> > > @@ -63,6 +63,7 @@ obj-y += wait.o
> > >  obj-bin-y += warning.init.o
> > >  obj-$(CONFIG_XENOPROF) += xenoprof.o
> > >  obj-y += xmalloc_tlsf.o
> > > +obj-y += numa.o
> >
> > This needs to be gated with CONFIG_NUMA.
> As it is shared with x86 and prior this changes it is not gated under
> any config for x86.
Because x86 code assume that CONFIG_NUMA is always enabled. If you look 
at the .config CONFIG_NUMA will be set so you can gate it here.
> > Looking at this comment, it looks like the NUMA support should depend on
> > HAS_PDX as this is not something that may be able on all the architecture.
> yes it uses pfn_to_pdx() while updating memnodemap.
> May be comment should be suffice.
A comment may be missed, gated in the Kconfig will prevent a new 
architecture to use NUMA without knowing PDX is necessary.
[...]

> > > +#endif /* CONFIG_NUMA */
> > > +
> > > +extern void numa_add_cpu(int cpu);
> > > +extern nodeid_t setup_node(unsigned int pxm);
> > > +extern void numa_set_node(int cpu, nodeid_t node);
> > > +extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
> >
> > I am not sure to understand why those function are not protected by #ifdef
> > CONFIFG_NUMA.
> As these defined in xen/common/numa.c which is not under CONFIG_NUMA,
> I have kept them outside CONFIG_NUMA
xen/common/numa.c should be under CONFIG_NUMA. There is no reason to 
expose the code on non-NUMA platform.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel