NUMA: promoting NUMA topology to BIOS and pin guest memory

PATCH In review Andre Przywara 3 participants 7 messages Dec 11, 2008 – Dec 14, 2008 Kbuild

Share & tools

Andre Przywara

author

2008-12-11 11:29 UTC #0 lore raw

PATCH 2/3 NUMA: promoting NUMA topology to BIOS and pin guest memory

This patch pushes the parsed NUMA topology via the firmware configuration interface to the BIOS and pins the guest memory (if desired).

Signed-off-by Andre Przywara <[email protected]>

Signature · 9 lines

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

# HG changeset patch
# User Andre Przywara <[email protected]> # Date 1228992161 -3600
# Node ID 0501b7490a00ef7a77e69f846d332f797162052a # Parent 394d02758aa4358be3bcd14f9d59efaf42e89328 promoting NUMA topology to BIOS and pin guest memory

Makefile.target +4 -0

diff -r 394d02758aa4 -r 0501b7490a00 Makefile.target
@@ -600,6 +600,10 @@ endif
 endif
 ifdef CONFIG_CS4231A
 SOUND_HW += cs4231a.o
+endif
+
+ifdef CONFIG_NUMA
+LIBS += -lnuma
 endif
 
 ifdef CONFIG_VNC_TLS

configure +23 -0

diff -r 394d02758aa4 -r 0501b7490a00 configure
@@ -368,6 +368,8 @@ for opt do
   ;;
   --enable-mixemu) mixemu="yes"
   ;;
+  --disable-numa) numa="no"
+  ;;
   --disable-aio) aio="no"
   ;;
   --disable-blobs) blobs="no"

@@ -462,6 +464,7 @@ echo "  --audio-card-list=LIST   set lis
 echo "  --audio-card-list=LIST   set list of additional emulated audio cards"
 echo "                           Available cards: ac97 adlib cs4231a gus"
 echo "  --enable-mixemu          enable mixer emulation"
+echo "  --disable-numa           disable NUMA support (host side)"
 echo "  --disable-brlapi         disable BrlAPI"
 echo "  --disable-vnc-tls        disable TLS encryption for VNC server"
 echo "  --disable-curses         disable curses output"

@@ -877,6 +880,21 @@ done
 done
 
 ##########################################
+# libnuma probe
+
+if test -z "$numa" ; then
+    numa=no
+
+    cat > $TMPC << EOF
+#include <numa.h>
+int main(void) { return numa_available(); }
+EOF
+    if $cc ${ARCH_CFLAGS} -o $TMPE ${OS_CFLAGS} $TMPC -lnuma > /dev/null 2> /dev/null ; then
+	    numa=yes
+    fi
+fi
+
+##########################################
 # BrlAPI probe
 
 if test -z "$brlapi" ; then

@@ -1033,6 +1051,7 @@ echo "Audio drivers     $audio_drv_list"
 echo "Audio drivers     $audio_drv_list"
 echo "Extra audio cards $audio_card_list"
 echo "Mixer emulation   $mixemu"
+echo "NUMA support      $numa"
 echo "VNC TLS support   $vnc_tls"
 if test "$vnc_tls" = "yes" ; then
     echo "    TLS CFLAGS    $vnc_tls_cflags"

@@ -1272,6 +1291,10 @@ if test "$mixemu" = "yes" ; then
 if test "$mixemu" = "yes" ; then
   echo "CONFIG_MIXEMU=yes" >> $config_mak
   echo "#define CONFIG_MIXEMU 1" >> $config_h
+fi
+if test "$numa" = "yes" ; then
+  echo "CONFIG_NUMA=yes" >> $config_mak
+  echo "#define CONFIG_NUMA 1" >> $config_h
 fi
 if test "$vnc_tls" = "yes" ; then
   echo "CONFIG_VNC_TLS=yes" >> $config_mak

hw/fw_cfg.h +3 -0

diff -r 394d02758aa4 -r 0501b7490a00 hw/fw_cfg.h
@@ -8,6 +8,9 @@
 #define FW_CFG_NOGRAPHIC        0x04
 #define FW_CFG_NB_CPUS          0x05
 #define FW_CFG_MACHINE_ID       0x06
+#define FW_CFG_NUMA_NODES       0x07
+#define FW_CFG_NUMA_NODE_CPUS   0x08
+#define FW_CFG_NUMA_NODE_MEM    0x09
 #define FW_CFG_MAX_ENTRY        0x10
 
 #define FW_CFG_WRITE_CHANNEL    0x4000

hw/pc.c +6 -0

diff -r 394d02758aa4 -r 0501b7490a00 hw/pc.c
@@ -436,6 +436,12 @@ static void bochs_bios_init(void)
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NUMA_NODES, numnumanodes);
+
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_MEM, (uint8_t*)node_mem,
+        sizeof(node_mem[0]) * numnumanodes);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_CPUS, (uint8_t*)node_to_cpus,
+        sizeof(node_to_cpus[0]) * numnumanodes);
 }
 
 /* Generate an initial boot sector which sets state and jump to

vl.c +18 -0

diff -r 394d02758aa4 -r 0501b7490a00 vl.c
@@ -93,6 +93,9 @@
 
 #include <linux/ppdev.h>
 #include <linux/parport.h>
+#ifdef CONFIG_NUMA
+#include <numa.h>
+#endif
 #endif
 #ifdef __sun__
 #include <sys/stat.h>

@@ -5449,6 +5452,21 @@ int main(int argc, char **argv, char **e
         exit(1);
     }
 
+#ifdef CONFIG_NUMA
+    if (numnumanodes > 0 && numa_available() != -1) {
+        unsigned long offset = 0;
+        int i;
+
+        for (i = 0; i < numnumanodes; ++i) {
+            if (hostnodes[i] != (uint64_t)-1) {
+                numa_tonode_memory (phys_ram_base + offset, node_mem[i],
+                    hostnodes[i] % (numa_max_node() + 1));
+            }
+            offset += node_mem[i];
+        }
+    }
+#endif
+
     /* init the dynamic translator */
     cpu_exec_init_all(tb_size * 1024 * 1024);

Anthony Liguori

2008-12-12 15:01 UTC #1 lore raw

Quoted 11 lines · Andre Przywara

This patch pushes the parsed NUMA topology via the firmware configuration interface to the BIOS and pins the guest memory (if desired).
Signed-off-by Andre Przywara <[email protected]>
# HG changeset patch
# User Andre Przywara <[email protected]> # Date 1228992161 -3600
# Node ID 0501b7490a00ef7a77e69f846d332f797162052a # Parent 394d02758aa4358be3bcd14f9d59efaf42e89328 promoting NUMA topology to BIOS and pin guest memory

Do you have a BIOS patch too?

Makefile.target endif · line 607

Quoted 12 lines

diff -r 394d02758aa4 -r 0501b7490a00 Makefile.target

Makefile.target +4 -0

@@ -600,6 +600,10 @@ endif
 endif
 ifdef CONFIG_CS4231A
 SOUND_HW += cs4231a.o
+endif
+
+ifdef CONFIG_NUMA
+LIBS += -lnuma
 endif

Should introduce NUMALIBS and set this from the configure as the rest of the library dependencies work.

Quoted 7 lines

Makefile.target +0 -0
 ifdef CONFIG_VNC_TLS
diff -r 394d02758aa4 -r 0501b7490a00 configure

configure for opt do · line 375

Quoted 8 lines

configure +2 -0

@@ -368,6 +368,8 @@ for opt do
   ;;
   --enable-mixemu) mixemu="yes"
   ;;
+  --disable-numa) numa="no"
+  ;;
   --disable-aio) aio="no"
   ;;
   --disable-blobs) blobs="no"

Need to set numa="yes" as a default.

Quoted 4 lines

diff -r 394d02758aa4 -r 0501b7490a00 hw/pc.c

hw/pc.c static void bochs_bios_init(void) · line 445

Quoted 10 lines

hw/pc.c +6 -0

@@ -436,6 +436,12 @@ static void bochs_bios_init(void)
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NUMA_NODES, numnumanodes);
+
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_MEM, (uint8_t*)node_mem,
+        sizeof(node_mem[0]) * numnumanodes);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_CPUS, (uint8_t*)node_to_cpus,
+        sizeof(node_to_cpus[0]) * numnumanodes);
 }

This stuff (the firmware awareness) should be independent of the libnuma support.

Quoted 24 lines

hw/pc.c +0 -0

 /* Generate an initial boot sector which sets state and jump to

diff -r 394d02758aa4 -r 0501b7490a00 vl.c

vl.c +8 -0

@@ -93,6 +93,9 @@
 
 #include <linux/ppdev.h>
 #include <linux/parport.h>
+#ifdef CONFIG_NUMA
+#include <numa.h>
+#endif
 #endif
 #ifdef __sun__
 #include <sys/stat.h>

@@ -5449,6 +5452,21 @@ int main(int argc, char **argv, char **e
         exit(1);
     }
 
+#ifdef CONFIG_NUMA
+    if (numnumanodes > 0 && numa_available() != -1) {
+        unsigned long offset = 0;
+        int i;
+

vl.c int main(int argc, char **argv, char **e · line 5469

Quoted 10 lines

vl.c +9 -0

+        for (i = 0; i < numnumanodes; ++i) {
+            if (hostnodes[i] != (uint64_t)-1) {
+                numa_tonode_memory (phys_ram_base + offset, node_mem[i],
+                    hostnodes[i] % (numa_max_node() + 1));
+            }
+            offset += node_mem[i];
+        }
+    }
+#endif

At this point, I'm okay with introducing the libnuma dependency for memory pinning. I'm not sure I think we should even present this as a command line option though. I think the command line option should just specify the NUMA topology and then we should use a monitor command for pinning (as you do on your next patch).

I'd like to see some more documentation of this functionality.

Regards,

Anthony Liguori

Quoted 3 lines

vl.c +0 -0

     /* init the dynamic translator */
     cpu_exec_init_all(tb_size * 1024 * 1024);

Avi Kivity

2008-12-13 09:42 UTC #2 lore raw

Quoted 2 lines · Anthony Liguori

At this point, I'm okay with introducing the libnuma dependency for memory pinning. I'm not sure I think we should even present this as a command line option though. I think the command line option should just specify the NUMA topology and then we should use a monitor command for pinning (as you do on your next patch).

It's helpful to have static pinning from the command line. That's useful for quick tests for those of us who don't use management interfaces.

Since the monitor and command line can share code, there shouldn't be any cost to this.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Andre Przywara

author

2008-12-13 23:18 UTC #3 lore raw

Quoted 7 lines · Avi Kivity

Quoted 2 lines · Anthony Liguori
At this point, I'm okay with introducing the libnuma dependency for memory pinning. I'm not sure I think we should even present this as a command line option though. I think the command line option should just specify the NUMA topology and then we should use a monitor command for pinning (as you do on your next patch).
It's helpful to have static pinning from the command line. That's useful for quick tests for those of us who don't use management interfaces.
Since the monitor and command line can share code, there shouldn't be any cost to this.

ACK. Actually I'd had to add code to prevent pinning from the command line. I think this doesn't hurt, if you use virtualization for partitioning (where a NUMA architecture can actually help you, because guests don't compete will all other guests for the memory bandwidth), it is quite helpful to specify everything at the beginning. Here even UP guest can take advantage:
-numa 1,pin:1

Regards,
Andre.

Signature · 9 lines

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Anthony Liguori

2008-12-13 23:42 UTC #4 lore raw

Quoted 9 lines · Andre Przywara

Quoted 7 lines · Avi Kivity
Quoted 2 lines · Anthony Liguori
At this point, I'm okay with introducing the libnuma dependency for memory pinning. I'm not sure I think we should even present this as a command line option though. I think the command line option should just specify the NUMA topology and then we should use a monitor command for pinning (as you do on your next patch).
It's helpful to have static pinning from the command line. That's useful for quick tests for those of us who don't use management interfaces.
Since the monitor and command line can share code, there shouldn't be any cost to this.
ACK. Actually I'd had to add code to prevent pinning from the command line. I think this doesn't hurt, if you use virtualization for partitioning (where a NUMA architecture can actually help you, because guests don't compete will all other guests for the memory bandwidth), it is quite helpful to specify everything at the beginning.

The thing that makes me uneasy about it is the fact that it's part of the base numa syntax. This combination of guest configuration and host configuration seems less than optimal to me. This isn't something that bothers me that much and won't keep me from applying the patches.

Regards,

Anthony Liguori

Quoted 6 lines

Here even UP guest can take advantage:
-numa 1,pin:1
Regards,
Andre.

Avi Kivity

2008-12-14 07:28 UTC #5 lore raw

Quoted 3 lines · Anthony Liguori

Quoted 1 line
ACK. Actually I'd had to add code to prevent pinning from the command line. I think this doesn't hurt, if you use virtualization for partitioning (where a NUMA architecture can actually help you, because guests don't compete will all other guests for the memory bandwidth), it is quite helpful to specify everything at the beginning.
The thing that makes me uneasy about it is the fact that it's part of the base numa syntax. This combination of guest configuration and host configuration seems less than optimal to me. This isn't something that bothers me that much and won't keep me from applying the patches.

I also think that it would be good to separate guest and host configuration, like -net nic and -net tap.

Signature · 1 line

error compiling committee.c: too many arguments to function

Andre Przywara

author

2008-12-13 23:26 UTC #6 lore raw

In reply to Anthony Liguori

Quoted 13 lines · Anthony Liguori

Quoted 11 lines · Andre Przywara
This patch pushes the parsed NUMA topology via the firmware configuration interface to the BIOS and pins the guest memory (if desired).
Signed-off-by Andre Przywara <[email protected]>
# HG changeset patch
# User Andre Przywara <[email protected]> # Date 1228992161 -3600
# Node ID 0501b7490a00ef7a77e69f846d332f797162052a # Parent 394d02758aa4358be3bcd14f9d59efaf42e89328 promoting NUMA topology to BIOS and pin guest memory

Quoted 1 line

Do you have a BIOS patch too?

Actually I was waiting with this part as you said you wanted to sync the QEMU's BIOS with the upstream BOCHS one, which would made my patch a lot easier. I can prepare a patch based on the current version in QEMU, but that diff would include code which is already in upstream BOCHS, which would complicate the next merge.

Quoted 17 lines

Quoted 7 lines
ifdef CONFIG_VNC_TLS
diff -r 394d02758aa4 -r 0501b7490a00 configure
configure Thu Dec 11 11:42:41 2008 +0100 for opt do · line 375
Quoted 8 lines
configure Thu Dec 11 11:42:41 2008 +0100 renamed from configure Thu Dec 11 11:36:21 2008 +0100 +2 -0
@@ -368,6 +368,8 @@ for opt do
   ;;
   --enable-mixemu) mixemu="yes"
   ;;
+  --disable-numa) numa="no"
+  ;;
   --disable-aio) aio="no"
   ;;
   --disable-blobs) blobs="no"
Need to set numa="yes" as a default.

Well, it seems there are two ways to do this in QEMU's configure:
1. (as in aio): Default to yes, optionally disable, if still set to yes compile check
2. (as in brlapi): no default, optionally set to "no", if empty string compile check and set to yes or no accordingly Seems like I copied the wrong version ;-)

Quoted 10 lines

Quoted 10 lines
diff -r 394d02758aa4 -r 0501b7490a00 hw/pc.c
hw/pc.c Thu Dec 11 11:42:41 2008 +0100 renamed from hw/pc.c Thu Dec 11 11:36:21 2008 +0100 +2 -0
@@ -436,6 +436,12 @@ static void bochs_bios_init(void)
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NUMA_NODES, numnumanodes);
+

Quoted 8 lines

hw/pc.c Thu Dec 11 11:42:41 2008 +0100 static void bochs_bios_init(void) · line 446
Quoted 6 lines
hw/pc.c Thu Dec 11 11:42:41 2008 +0100 renamed from hw/pc.c Thu Dec 11 11:36:21 2008 +0100 +4 -0
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_MEM, (uint8_t*)node_mem,
+        sizeof(node_mem[0]) * numnumanodes);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_CPUS, 
 (uint8_t*)node_to_cpus,
+        sizeof(node_to_cpus[0]) * numnumanodes);
 }
This stuff (the firmware awareness) should be independent of the libnuma support.

Moved to the first patch.

Thanks for the detailed review. I will address your other comments as well.

Regards,
Andre.

Signature · 9 lines

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy