Re: [Xen-devel] [RFC PATCH v2 09/25] ARM: NUMA: Add existing ARM numa code under CONFIG_NUMA

[Julien Grall <julien.grall@xxxxxxx>]

On 05/09/2017 08:14 AM, Vijay Kilari wrote:
> On Mon, May 8, 2017 at 9:28 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Vijay,
> >
> > On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
> > > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > >
> > > Right now CONFIG_NUMA is not enabled for ARM and
> > > existing code in asm-arm/numa.h is for !CONFIG_NUMA.
> > > Hence put this code under #ifndef CONFIG_NUMA.
> > >
> > > This help to make this changes work when CONFIG_NUMA
> > > is not enabled.
> >
> > But you always turn NUMA on by default (see patch #24) and there is no
> > possibility to turn off NUMA.
> Yes at the end of the series we enable NUMA by default.
> But the the intermittent patches of this patch series fails to compile.
So for helping this series, you add code that will get rotten???

I don't like this idea at all, we should avoid to add code in Xen that 
will not be used.
> > > Also define NODES_SHIFT macro for ARM to value 2.
> > > This limits number of NUMA nodes supported to 4.
> > > There is not hard restrictions on this value set to 2.
> >
> > Again, why only 2 when x86 is supporting 6?
> >
> > Furthermore, this is not related to this patch itself and should be part of
> > separate patch.
> >
> > Lastly, why don't you move that to a Kconfig allowing the user to configure
> > the number of Nodes?
> ok
>
> > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > > ---
> > >  xen/include/asm-arm/numa.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > > index 53f99af..924bfc0 100644
> > > --- a/xen/include/asm-arm/numa.h
> > > +++ b/xen/include/asm-arm/numa.h
> > > @@ -3,6 +3,10 @@
> > >
> > >  typedef uint8_t nodeid_t;
> > >
> > > +/* Limit number of NUMA nodes supported to 4 */
> > > +#define NODES_SHIFT 2
> >
> > Why this is not covered by CONFIG_NUMA?
> The below define is used in generic code irrespective of CONFIG_NUMA
>
> #define MAX_NUMNODES    (1 << NODES_SHIFT)
As you may have noticed NODES_SHIFT currently does not exist on ARM and 
we are still able to compile the generic code. So why do you need to do 
it unconditionally?
If you look at the code, xen/numa.h will define NODES_SHIFT to 0 if it 
has not previously defined. So I still don't see any reason on what you 
are doing.
Cheers,

--
Julien Grall

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