[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.