Re: [Xen-devel] [RFC PATCH v3 01/24] NUMA: Make number of NUMA nodes configurable

[Julien Grall <julien.grall@xxxxxxx>]

Hi Vijay,

On 19/07/2017 08:00, Vijay Kilari wrote:
> On Tue, Jul 18, 2017 at 11:25 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi,
> >
> >
> > On 18/07/17 12:41, vijay.kilari@xxxxxxxxx wrote:
> > > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > >
> > > Introduce NR_NODES config option to specify number
> > > of NUMA nodes supported. By default value is set at
> > > 64 for x86 and 8 for arm. Dropped NODES_SHIFT macro.
> > >
> > > Also move NR_NODE_MEMBLKS from asm-x86/acpi.h to xen/numa.h
> > >
> > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > > ---
> > >  xen/arch/Kconfig           | 7 +++++++
> > >  xen/include/asm-x86/acpi.h | 1 -
> > >  xen/include/asm-x86/numa.h | 2 --
> > >  xen/include/xen/config.h   | 1 +
> > >  xen/include/xen/numa.h     | 7 ++-----
> > >  5 files changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> > > index cf0acb7..9c2a4e2 100644
> > > --- a/xen/arch/Kconfig
> > > +++ b/xen/arch/Kconfig
> > > @@ -6,3 +6,10 @@ config NR_CPUS
> > >         default "128" if ARM
> > >         ---help---
> > >           Specifies the maximum number of physical CPUs which Xen will
> > > support.
> > > +
> > > +config NR_NODES
> > > +       int "Maximum number of NUMA nodes"
> > > +       default "64" if X86
> > > +       default "8" if ARM
> >
> > 3rd time I am asking it... Why the difference between x86 and ARM?
> AFAIK, there is no arm platform for now with numa more than 8 nodes.
> Thunderx is only 2 nodes.
> So kept it low value for ARM to avoid unnecessary memory allocation.
>
> Do you want me to keep same as x86?.
Well, you say it is for saving memory allocation but you don't give any 
number on how much you can save by reducing the default from 64 to 8...
Looking at it, MAX_NUMNODES is used for some static allocation and also 
for the bitmap nodemask_t.
Because our bitmap is based on unsigned long, you would use the same 
quantity of memory for AArch64, for AArch32 the quantity will be divided 
by two. Still nodemask_t does not seem to be widely used.
In the case of the static allocation, I spot ~40 bytes per NUMA node. So 
8 node will use ~320 bytes and 64 bytes ~2560.
NUMA is likely going to be used in server, don't tell me you are 2k 
short in memory? If it is an issue it is better to think how to limit 
the number of static variable rather than putting a low limit here.
For Embedded use case, they will likely want to put the default to 1 but 
I would not worry about them as they are likely going to tweak the Kconfig.
> > Also, you likely want to set to 1 if NUMA is not enabled.
> I don't see any dependency of NR_NODES with NUMA config.
> So it is always set to default value. Isn't?
Well, what is the point to allow more than 1 node when NUMA is not 
supported?
Not mentioning that this is quite confusing for a user to allow setting 
up the maximum number of nodes if the archicture is not supporting numa...
For instance, this is the case today on ARM because, without this 
series, we don't support NUMA.
> > > +       ---help---
> > > +         Specifies the maximum number of NUMA nodes which Xen will
> > > support.
> > > diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
> > > index 27ecc65..15be784 100644
> > > --- a/xen/include/asm-x86/acpi.h
> > > +++ b/xen/include/asm-x86/acpi.h
> > > @@ -105,7 +105,6 @@ extern void acpi_reserve_bootmem(void);
> > >
> > >  extern s8 acpi_numa;
> > >  extern int acpi_scan_nodes(u64 start, u64 end);
> > > -#define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
> > >
> > >  #ifdef CONFIG_ACPI_SLEEP
> > >
> > > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> > > index bada2c0..3cf26c2 100644
> > > --- a/xen/include/asm-x86/numa.h
> > > +++ b/xen/include/asm-x86/numa.h
> > > @@ -3,8 +3,6 @@
> > >
> > >  #include <xen/cpumask.h>
> > >
> > > -#define NODES_SHIFT 6
> > > -
> > >  typedef u8 nodeid_t;
> > >
> > >  extern int srat_rev;
> > > diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
> > > index a1d0f97..0f1a029 100644
> > > --- a/xen/include/xen/config.h
> > > +++ b/xen/include/xen/config.h
> > > @@ -81,6 +81,7 @@
> > >
> > >  /* allow existing code to work with Kconfig variable */
> > >  #define NR_CPUS CONFIG_NR_CPUS
> > > +#define NR_NODES CONFIG_NR_NODES
> > >
> > >  #ifndef CONFIG_DEBUG
> > >  #define NDEBUG
> > > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> > > index 7aef1a8..6bba29e 100644
> > > --- a/xen/include/xen/numa.h
> > > +++ b/xen/include/xen/numa.h
> > > @@ -3,14 +3,11 @@
> > >
> > >  #include <asm/numa.h>
> > >
> > > -#ifndef NODES_SHIFT
> > > -#define NODES_SHIFT     0
> > > -#endif
> > > -
> > >  #define NUMA_NO_NODE     0xFF
> > >  #define NUMA_NO_DISTANCE 0xFF
> > >
> > > -#define MAX_NUMNODES    (1 << NODES_SHIFT)
> > > +#define MAX_NUMNODES    NR_NODES
> > > +#define NR_NODE_MEMBLKS (MAX_NUMNODES * 2)
Also, I don't understand why you move this define from asm-x86/numa.h to 
xen/numa.h. At least, this does not seem related to this patch...
Cheers,


--
Julien Grall

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