* [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
@ 2013-11-18 21:58 ` Elena Ufimtseva
2013-11-18 21:58 ` Elena Ufimtseva
` (4 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Elena Ufimtseva @ 2013-11-18 21:58 UTC (permalink / raw)
To: xen-devel
Cc: akpm, wency, x86, linux-kernel, tangchen, mingo, david.vrabel,
Elena Ufimtseva, hpa, boris.ostrovsky, tglx, stefano.stabellini,
ian.campbell
Issues Xen hypercall subop XENMEM_get_vnumainfo and sets the
NUMA topology, otherwise sets dummy NUMA node and prevents
numa_init from calling other numa initializators as they dont
work with pv guests.
Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
---
arch/x86/include/asm/xen/vnuma.h | 12 ++++
arch/x86/mm/numa.c | 3 +
arch/x86/xen/Makefile | 2 +-
arch/x86/xen/vnuma.c | 127 ++++++++++++++++++++++++++++++++++++++
include/xen/interface/memory.h | 43 +++++++++++++
5 files changed, 186 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/include/asm/xen/vnuma.h
create mode 100644 arch/x86/xen/vnuma.c
diff --git a/arch/x86/include/asm/xen/vnuma.h b/arch/x86/include/asm/xen/vnuma.h
new file mode 100644
index 0000000..aee4e92
--- /dev/null
+++ b/arch/x86/include/asm/xen/vnuma.h
@@ -0,0 +1,12 @@
+#ifndef _ASM_X86_VNUMA_H
+#define _ASM_X86_VNUMA_H
+
+#ifdef CONFIG_XEN
+bool xen_vnuma_supported(void);
+int xen_numa_init(void);
+#else
+static inline bool xen_vnuma_supported(void) { return false; };
+static inline int xen_numa_init(void) { return -1; };
+#endif
+
+#endif /* _ASM_X86_VNUMA_H */
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index 24aec58..99efa1b 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -17,6 +17,7 @@
#include <asm/dma.h>
#include <asm/acpi.h>
#include <asm/amd_nb.h>
+#include "asm/xen/vnuma.h"
#include "numa_internal.h"
@@ -632,6 +633,8 @@ static int __init dummy_numa_init(void)
void __init x86_numa_init(void)
{
if (!numa_off) {
+ if (!numa_init(xen_numa_init))
+ return;
#ifdef CONFIG_X86_NUMAQ
if (!numa_init(numaq_numa_init))
return;
diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index 96ab2c0..de9deab 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -13,7 +13,7 @@ CFLAGS_mmu.o := $(nostackp)
obj-y := enlighten.o setup.o multicalls.o mmu.o irq.o \
time.o xen-asm.o xen-asm_$(BITS).o \
grant-table.o suspend.o platform-pci-unplug.o \
- p2m.o
+ p2m.o vnuma.o
obj-$(CONFIG_EVENT_TRACING) += trace.o
diff --git a/arch/x86/xen/vnuma.c b/arch/x86/xen/vnuma.c
new file mode 100644
index 0000000..caa2178
--- /dev/null
+++ b/arch/x86/xen/vnuma.c
@@ -0,0 +1,127 @@
+#include <linux/err.h>
+#include <linux/memblock.h>
+#include <xen/interface/xen.h>
+#include <xen/interface/memory.h>
+#include <asm/xen/interface.h>
+#include <asm/xen/hypercall.h>
+#include <asm/xen/vnuma.h>
+
+#ifdef CONFIG_NUMA
+
+/* Checks if hypercall is supported */
+bool xen_vnuma_supported(void)
+{
+ return HYPERVISOR_memory_op(XENMEM_get_vnuma_info, NULL)
+ == -ENOSYS ? false : true;
+}
+
+/*
+ * Called from numa_init if numa_off = 0;
+ * we set numa_off = 0 if xen_vnuma_supported()
+ * returns true and its a domU;
+ */
+int __init xen_numa_init(void)
+{
+ int rc;
+ unsigned int i, j, nr_nodes, cpu, idx, pcpus;
+ u64 physm, physd, physc;
+ unsigned int *vdistance, *cpu_to_node;
+ unsigned long mem_size, dist_size, cpu_to_node_size;
+ struct vmemrange *vblock;
+
+ struct vnuma_topology_info numa_topo = {
+ .domid = DOMID_SELF,
+ .__pad = 0
+ };
+ rc = -EINVAL;
+ physm = physd = physc = 0;
+
+ /* For now only PV guests are supported */
+ if (!xen_pv_domain())
+ return rc;
+
+ pcpus = num_possible_cpus();
+
+ mem_size = pcpus * sizeof(struct vmemrange);
+ dist_size = pcpus * pcpus * sizeof(*numa_topo.distance);
+ cpu_to_node_size = pcpus * sizeof(*numa_topo.cpu_to_node);
+
+ physm = memblock_alloc(mem_size, PAGE_SIZE);
+ vblock = __va(physm);
+
+ physd = memblock_alloc(dist_size, PAGE_SIZE);
+ vdistance = __va(physd);
+
+ physc = memblock_alloc(cpu_to_node_size, PAGE_SIZE);
+ cpu_to_node = __va(physc);
+
+ if (!physm || !physc || !physd)
+ goto out;
+
+ set_xen_guest_handle(numa_topo.nr_nodes, &nr_nodes);
+ set_xen_guest_handle(numa_topo.memrange, vblock);
+ set_xen_guest_handle(numa_topo.distance, vdistance);
+ set_xen_guest_handle(numa_topo.cpu_to_node, cpu_to_node);
+
+ rc = HYPERVISOR_memory_op(XENMEM_get_vnuma_info, &numa_topo);
+
+ if (rc < 0)
+ goto out;
+ nr_nodes = *numa_topo.nr_nodes;
+ if (nr_nodes == 0)
+ goto out;
+ if (nr_nodes > num_possible_cpus()) {
+ pr_debug("vNUMA: Node without cpu is not supported in this version.\n");
+ goto out;
+ }
+
+ /*
+ * NUMA nodes memory ranges are in pfns, constructed and
+ * aligned based on e820 ram domain map.
+ */
+ for (i = 0; i < nr_nodes; i++) {
+ if (numa_add_memblk(i, vblock[i].start, vblock[i].end))
+ goto out;
+ node_set(i, numa_nodes_parsed);
+ }
+
+ setup_nr_node_ids();
+ /* Setting the cpu, apicid to node */
+ for_each_cpu(cpu, cpu_possible_mask) {
+ set_apicid_to_node(cpu, cpu_to_node[cpu]);
+ numa_set_node(cpu, cpu_to_node[cpu]);
+ cpumask_set_cpu(cpu, node_to_cpumask_map[cpu_to_node[cpu]]);
+ }
+
+ for (i = 0; i < nr_nodes; i++) {
+ for (j = 0; j < *numa_topo.nr_nodes; j++) {
+ idx = (j * nr_nodes) + i;
+ numa_set_distance(i, j, *(vdistance + idx));
+ }
+ }
+
+ rc = 0;
+out:
+ if (physm)
+ memblock_free(__pa(physm), mem_size);
+ if (physd)
+ memblock_free(__pa(physd), dist_size);
+ if (physc)
+ memblock_free(__pa(physc), cpu_to_node_size);
+ /*
+ * Set a dummy node and return success. This prevents calling any
+ * hardware-specific initializers which do not work in a PV guest.
+ * Taken from dummy_numa_init code.
+ */
+ if (rc != 0) {
+ for (i = 0; i < MAX_LOCAL_APIC; i++)
+ set_apicid_to_node(i, NUMA_NO_NODE);
+ nodes_clear(numa_nodes_parsed);
+ nodes_clear(node_possible_map);
+ nodes_clear(node_online_map);
+ node_set(0, numa_nodes_parsed);
+ numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
+ }
+ return 0;
+}
+#endif
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 2ecfe4f..94311ee 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -263,4 +263,47 @@ struct xen_remove_from_physmap {
};
DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
+/* vNUMA structures */
+struct vmemrange {
+ uint64_t start, end;
+ /* reserved */
+ uint64_t _padm;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vmemrange);
+
+struct vnuma_topology_info {
+ /* OUT */
+ domid_t domid;
+ uint32_t __pad;
+ /* IN */
+ /* number of virtual numa nodes */
+ union {
+ GUEST_HANDLE(uint) nr_nodes;
+ uint64_t _padn;
+ };
+ /* distance table */
+ union {
+ GUEST_HANDLE(uint) distance;
+ uint64_t _padd;
+ };
+ /* cpu mapping to vnodes */
+ union {
+ GUEST_HANDLE(uint) cpu_to_node;
+ uint64_t _padc;
+ };
+ /*
+ * memory areas constructed by Xen, start and end
+ * of the ranges are specific to domain e820 map.
+ * Xen toolstack constructs these ranges for domain
+ * when building it.
+ */
+ union {
+ GUEST_HANDLE(vmemrange) memrange;
+ uint64_t _padm;
+ };
+};
+DEFINE_GUEST_HANDLE_STRUCT(vnuma_topology_info);
+
+#define XENMEM_get_vnuma_info 25
+
#endif /* __XEN_PUBLIC_MEMORY_H__ */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
2013-11-18 21:58 ` [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU Elena Ufimtseva
@ 2013-11-18 21:58 ` Elena Ufimtseva
2013-11-19 11:53 ` David Vrabel
2013-11-19 11:53 ` David Vrabel
2013-11-18 21:58 ` [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest Elena Ufimtseva
` (3 subsequent siblings)
5 siblings, 2 replies; 27+ messages in thread
From: Elena Ufimtseva @ 2013-11-18 21:58 UTC (permalink / raw)
To: xen-devel
Cc: konrad.wilk, boris.ostrovsky, david.vrabel, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel, Elena Ufimtseva
Issues Xen hypercall subop XENMEM_get_vnumainfo and sets the
NUMA topology, otherwise sets dummy NUMA node and prevents
numa_init from calling other numa initializators as they dont
work with pv guests.
Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
---
arch/x86/include/asm/xen/vnuma.h | 12 ++++
arch/x86/mm/numa.c | 3 +
arch/x86/xen/Makefile | 2 +-
arch/x86/xen/vnuma.c | 127 ++++++++++++++++++++++++++++++++++++++
include/xen/interface/memory.h | 43 +++++++++++++
5 files changed, 186 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/include/asm/xen/vnuma.h
create mode 100644 arch/x86/xen/vnuma.c
diff --git a/arch/x86/include/asm/xen/vnuma.h b/arch/x86/include/asm/xen/vnuma.h
new file mode 100644
index 0000000..aee4e92
--- /dev/null
+++ b/arch/x86/include/asm/xen/vnuma.h
@@ -0,0 +1,12 @@
+#ifndef _ASM_X86_VNUMA_H
+#define _ASM_X86_VNUMA_H
+
+#ifdef CONFIG_XEN
+bool xen_vnuma_supported(void);
+int xen_numa_init(void);
+#else
+static inline bool xen_vnuma_supported(void) { return false; };
+static inline int xen_numa_init(void) { return -1; };
+#endif
+
+#endif /* _ASM_X86_VNUMA_H */
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index 24aec58..99efa1b 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -17,6 +17,7 @@
#include <asm/dma.h>
#include <asm/acpi.h>
#include <asm/amd_nb.h>
+#include "asm/xen/vnuma.h"
#include "numa_internal.h"
@@ -632,6 +633,8 @@ static int __init dummy_numa_init(void)
void __init x86_numa_init(void)
{
if (!numa_off) {
+ if (!numa_init(xen_numa_init))
+ return;
#ifdef CONFIG_X86_NUMAQ
if (!numa_init(numaq_numa_init))
return;
diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index 96ab2c0..de9deab 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -13,7 +13,7 @@ CFLAGS_mmu.o := $(nostackp)
obj-y := enlighten.o setup.o multicalls.o mmu.o irq.o \
time.o xen-asm.o xen-asm_$(BITS).o \
grant-table.o suspend.o platform-pci-unplug.o \
- p2m.o
+ p2m.o vnuma.o
obj-$(CONFIG_EVENT_TRACING) += trace.o
diff --git a/arch/x86/xen/vnuma.c b/arch/x86/xen/vnuma.c
new file mode 100644
index 0000000..caa2178
--- /dev/null
+++ b/arch/x86/xen/vnuma.c
@@ -0,0 +1,127 @@
+#include <linux/err.h>
+#include <linux/memblock.h>
+#include <xen/interface/xen.h>
+#include <xen/interface/memory.h>
+#include <asm/xen/interface.h>
+#include <asm/xen/hypercall.h>
+#include <asm/xen/vnuma.h>
+
+#ifdef CONFIG_NUMA
+
+/* Checks if hypercall is supported */
+bool xen_vnuma_supported(void)
+{
+ return HYPERVISOR_memory_op(XENMEM_get_vnuma_info, NULL)
+ == -ENOSYS ? false : true;
+}
+
+/*
+ * Called from numa_init if numa_off = 0;
+ * we set numa_off = 0 if xen_vnuma_supported()
+ * returns true and its a domU;
+ */
+int __init xen_numa_init(void)
+{
+ int rc;
+ unsigned int i, j, nr_nodes, cpu, idx, pcpus;
+ u64 physm, physd, physc;
+ unsigned int *vdistance, *cpu_to_node;
+ unsigned long mem_size, dist_size, cpu_to_node_size;
+ struct vmemrange *vblock;
+
+ struct vnuma_topology_info numa_topo = {
+ .domid = DOMID_SELF,
+ .__pad = 0
+ };
+ rc = -EINVAL;
+ physm = physd = physc = 0;
+
+ /* For now only PV guests are supported */
+ if (!xen_pv_domain())
+ return rc;
+
+ pcpus = num_possible_cpus();
+
+ mem_size = pcpus * sizeof(struct vmemrange);
+ dist_size = pcpus * pcpus * sizeof(*numa_topo.distance);
+ cpu_to_node_size = pcpus * sizeof(*numa_topo.cpu_to_node);
+
+ physm = memblock_alloc(mem_size, PAGE_SIZE);
+ vblock = __va(physm);
+
+ physd = memblock_alloc(dist_size, PAGE_SIZE);
+ vdistance = __va(physd);
+
+ physc = memblock_alloc(cpu_to_node_size, PAGE_SIZE);
+ cpu_to_node = __va(physc);
+
+ if (!physm || !physc || !physd)
+ goto out;
+
+ set_xen_guest_handle(numa_topo.nr_nodes, &nr_nodes);
+ set_xen_guest_handle(numa_topo.memrange, vblock);
+ set_xen_guest_handle(numa_topo.distance, vdistance);
+ set_xen_guest_handle(numa_topo.cpu_to_node, cpu_to_node);
+
+ rc = HYPERVISOR_memory_op(XENMEM_get_vnuma_info, &numa_topo);
+
+ if (rc < 0)
+ goto out;
+ nr_nodes = *numa_topo.nr_nodes;
+ if (nr_nodes == 0)
+ goto out;
+ if (nr_nodes > num_possible_cpus()) {
+ pr_debug("vNUMA: Node without cpu is not supported in this version.\n");
+ goto out;
+ }
+
+ /*
+ * NUMA nodes memory ranges are in pfns, constructed and
+ * aligned based on e820 ram domain map.
+ */
+ for (i = 0; i < nr_nodes; i++) {
+ if (numa_add_memblk(i, vblock[i].start, vblock[i].end))
+ goto out;
+ node_set(i, numa_nodes_parsed);
+ }
+
+ setup_nr_node_ids();
+ /* Setting the cpu, apicid to node */
+ for_each_cpu(cpu, cpu_possible_mask) {
+ set_apicid_to_node(cpu, cpu_to_node[cpu]);
+ numa_set_node(cpu, cpu_to_node[cpu]);
+ cpumask_set_cpu(cpu, node_to_cpumask_map[cpu_to_node[cpu]]);
+ }
+
+ for (i = 0; i < nr_nodes; i++) {
+ for (j = 0; j < *numa_topo.nr_nodes; j++) {
+ idx = (j * nr_nodes) + i;
+ numa_set_distance(i, j, *(vdistance + idx));
+ }
+ }
+
+ rc = 0;
+out:
+ if (physm)
+ memblock_free(__pa(physm), mem_size);
+ if (physd)
+ memblock_free(__pa(physd), dist_size);
+ if (physc)
+ memblock_free(__pa(physc), cpu_to_node_size);
+ /*
+ * Set a dummy node and return success. This prevents calling any
+ * hardware-specific initializers which do not work in a PV guest.
+ * Taken from dummy_numa_init code.
+ */
+ if (rc != 0) {
+ for (i = 0; i < MAX_LOCAL_APIC; i++)
+ set_apicid_to_node(i, NUMA_NO_NODE);
+ nodes_clear(numa_nodes_parsed);
+ nodes_clear(node_possible_map);
+ nodes_clear(node_online_map);
+ node_set(0, numa_nodes_parsed);
+ numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
+ }
+ return 0;
+}
+#endif
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 2ecfe4f..94311ee 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -263,4 +263,47 @@ struct xen_remove_from_physmap {
};
DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
+/* vNUMA structures */
+struct vmemrange {
+ uint64_t start, end;
+ /* reserved */
+ uint64_t _padm;
+};
+DEFINE_GUEST_HANDLE_STRUCT(vmemrange);
+
+struct vnuma_topology_info {
+ /* OUT */
+ domid_t domid;
+ uint32_t __pad;
+ /* IN */
+ /* number of virtual numa nodes */
+ union {
+ GUEST_HANDLE(uint) nr_nodes;
+ uint64_t _padn;
+ };
+ /* distance table */
+ union {
+ GUEST_HANDLE(uint) distance;
+ uint64_t _padd;
+ };
+ /* cpu mapping to vnodes */
+ union {
+ GUEST_HANDLE(uint) cpu_to_node;
+ uint64_t _padc;
+ };
+ /*
+ * memory areas constructed by Xen, start and end
+ * of the ranges are specific to domain e820 map.
+ * Xen toolstack constructs these ranges for domain
+ * when building it.
+ */
+ union {
+ GUEST_HANDLE(vmemrange) memrange;
+ uint64_t _padm;
+ };
+};
+DEFINE_GUEST_HANDLE_STRUCT(vnuma_topology_info);
+
+#define XENMEM_get_vnuma_info 25
+
#endif /* __XEN_PUBLIC_MEMORY_H__ */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU
2013-11-18 21:58 ` Elena Ufimtseva
@ 2013-11-19 11:53 ` David Vrabel
2013-11-19 11:53 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 11:53 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: akpm, wency, x86, linux-kernel, tangchen, mingo, hpa, xen-devel,
boris.ostrovsky, tglx, stefano.stabellini, ian.campbell
On 18/11/13 21:58, Elena Ufimtseva wrote:
> Issues Xen hypercall subop XENMEM_get_vnumainfo and sets the
> NUMA topology, otherwise sets dummy NUMA node and prevents
> numa_init from calling other numa initializators as they dont
> work with pv guests.
[...]
> --- /dev/null
> +++ b/arch/x86/include/asm/xen/vnuma.h
> @@ -0,0 +1,12 @@
> +#ifndef _ASM_X86_VNUMA_H
> +#define _ASM_X86_VNUMA_H
> +
> +#ifdef CONFIG_XEN
> +bool xen_vnuma_supported(void);
> +int xen_numa_init(void);
> +#else
> +static inline bool xen_vnuma_supported(void) { return false; };
> +static inline int xen_numa_init(void) { return -1; };
Return a valid -ve errno here. e.g., -ENOSYS.
> +#endif
> +
> +#endif /* _ASM_X86_VNUMA_H */
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 24aec58..99efa1b 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -17,6 +17,7 @@
> #include <asm/dma.h>
> #include <asm/acpi.h>
> #include <asm/amd_nb.h>
> +#include "asm/xen/vnuma.h"
>
> #include "numa_internal.h"
>
> @@ -632,6 +633,8 @@ static int __init dummy_numa_init(void)
> void __init x86_numa_init(void)
> {
> if (!numa_off) {
> + if (!numa_init(xen_numa_init))
> + return;
> #ifdef CONFIG_X86_NUMAQ
> if (!numa_init(numaq_numa_init))
> return;
> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
> index 96ab2c0..de9deab 100644
> --- a/arch/x86/xen/Makefile
> +++ b/arch/x86/xen/Makefile
> @@ -13,7 +13,7 @@ CFLAGS_mmu.o := $(nostackp)
> obj-y := enlighten.o setup.o multicalls.o mmu.o irq.o \
> time.o xen-asm.o xen-asm_$(BITS).o \
> grant-table.o suspend.o platform-pci-unplug.o \
> - p2m.o
> + p2m.o vnuma.o
obj-$(CONFIG_NUMA) += vnuma.o
I commented on this already, please take more care to address all
comments before reposting.
> --- /dev/null
> +++ b/arch/x86/xen/vnuma.c
> @@ -0,0 +1,127 @@
> +#include <linux/err.h>
> +#include <linux/memblock.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/memory.h>
> +#include <asm/xen/interface.h>
> +#include <asm/xen/hypercall.h>
> +#include <asm/xen/vnuma.h>
> +
> +#ifdef CONFIG_NUMA
> +
> +/* Checks if hypercall is supported */
> +bool xen_vnuma_supported(void)
> +{
> + return HYPERVISOR_memory_op(XENMEM_get_vnuma_info, NULL)
> + == -ENOSYS ? false : true;
return HYPERVISOR_memory_op() != -ENOSYS;
But as I note in the other email, I think this function isn't needed.
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -263,4 +263,47 @@ struct xen_remove_from_physmap {
> };
> DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
>
> +/* vNUMA structures */
> +struct vmemrange {
> + uint64_t start, end;
> + /* reserved */
> + uint64_t _padm;
If this field is reserved for future use, please name it _reserved and
ensure that the hypervisor checks that it is 0.
> +struct vnuma_topology_info {
> + /* OUT */
> + domid_t domid;
> + uint32_t __pad;
> + /* IN */
> + /* number of virtual numa nodes */
> + union {
> + GUEST_HANDLE(uint) nr_nodes;
> + uint64_t _padn;
> + };
I don't think anonymous unions are allowed in the ABI. They're a c99
extension I think.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU
2013-11-18 21:58 ` Elena Ufimtseva
2013-11-19 11:53 ` David Vrabel
@ 2013-11-19 11:53 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 11:53 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: xen-devel, konrad.wilk, boris.ostrovsky, tglx, mingo, hpa, x86,
akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On 18/11/13 21:58, Elena Ufimtseva wrote:
> Issues Xen hypercall subop XENMEM_get_vnumainfo and sets the
> NUMA topology, otherwise sets dummy NUMA node and prevents
> numa_init from calling other numa initializators as they dont
> work with pv guests.
[...]
> --- /dev/null
> +++ b/arch/x86/include/asm/xen/vnuma.h
> @@ -0,0 +1,12 @@
> +#ifndef _ASM_X86_VNUMA_H
> +#define _ASM_X86_VNUMA_H
> +
> +#ifdef CONFIG_XEN
> +bool xen_vnuma_supported(void);
> +int xen_numa_init(void);
> +#else
> +static inline bool xen_vnuma_supported(void) { return false; };
> +static inline int xen_numa_init(void) { return -1; };
Return a valid -ve errno here. e.g., -ENOSYS.
> +#endif
> +
> +#endif /* _ASM_X86_VNUMA_H */
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 24aec58..99efa1b 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -17,6 +17,7 @@
> #include <asm/dma.h>
> #include <asm/acpi.h>
> #include <asm/amd_nb.h>
> +#include "asm/xen/vnuma.h"
>
> #include "numa_internal.h"
>
> @@ -632,6 +633,8 @@ static int __init dummy_numa_init(void)
> void __init x86_numa_init(void)
> {
> if (!numa_off) {
> + if (!numa_init(xen_numa_init))
> + return;
> #ifdef CONFIG_X86_NUMAQ
> if (!numa_init(numaq_numa_init))
> return;
> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
> index 96ab2c0..de9deab 100644
> --- a/arch/x86/xen/Makefile
> +++ b/arch/x86/xen/Makefile
> @@ -13,7 +13,7 @@ CFLAGS_mmu.o := $(nostackp)
> obj-y := enlighten.o setup.o multicalls.o mmu.o irq.o \
> time.o xen-asm.o xen-asm_$(BITS).o \
> grant-table.o suspend.o platform-pci-unplug.o \
> - p2m.o
> + p2m.o vnuma.o
obj-$(CONFIG_NUMA) += vnuma.o
I commented on this already, please take more care to address all
comments before reposting.
> --- /dev/null
> +++ b/arch/x86/xen/vnuma.c
> @@ -0,0 +1,127 @@
> +#include <linux/err.h>
> +#include <linux/memblock.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/memory.h>
> +#include <asm/xen/interface.h>
> +#include <asm/xen/hypercall.h>
> +#include <asm/xen/vnuma.h>
> +
> +#ifdef CONFIG_NUMA
> +
> +/* Checks if hypercall is supported */
> +bool xen_vnuma_supported(void)
> +{
> + return HYPERVISOR_memory_op(XENMEM_get_vnuma_info, NULL)
> + == -ENOSYS ? false : true;
return HYPERVISOR_memory_op() != -ENOSYS;
But as I note in the other email, I think this function isn't needed.
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -263,4 +263,47 @@ struct xen_remove_from_physmap {
> };
> DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
>
> +/* vNUMA structures */
> +struct vmemrange {
> + uint64_t start, end;
> + /* reserved */
> + uint64_t _padm;
If this field is reserved for future use, please name it _reserved and
ensure that the hypervisor checks that it is 0.
> +struct vnuma_topology_info {
> + /* OUT */
> + domid_t domid;
> + uint32_t __pad;
> + /* IN */
> + /* number of virtual numa nodes */
> + union {
> + GUEST_HANDLE(uint) nr_nodes;
> + uint64_t _padn;
> + };
I don't think anonymous unions are allowed in the ABI. They're a c99
extension I think.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
2013-11-18 21:58 ` [PATCH RESEND v2 1/2] xen: vnuma support for PV guests running as domU Elena Ufimtseva
2013-11-18 21:58 ` Elena Ufimtseva
@ 2013-11-18 21:58 ` Elena Ufimtseva
2013-11-18 21:58 ` Elena Ufimtseva
` (2 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Elena Ufimtseva @ 2013-11-18 21:58 UTC (permalink / raw)
To: xen-devel
Cc: akpm, wency, x86, linux-kernel, tangchen, mingo, david.vrabel,
Elena Ufimtseva, hpa, boris.ostrovsky, tglx, stefano.stabellini,
ian.campbell
Enables numa if vnuma topology hypercall is supported and it is domU.
Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
---
arch/x86/xen/setup.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 68c054f..0aab799 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -20,6 +20,7 @@
#include <asm/numa.h>
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
+#include <asm/xen/vnuma.h>
#include <xen/xen.h>
#include <xen/page.h>
@@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
WARN_ON(xen_set_default_idle());
fiddle_vdso();
#ifdef CONFIG_NUMA
- numa_off = 1;
+ if (!xen_initial_domain() && xen_vnuma_supported())
+ numa_off = 0;
+ else
+ numa_off = 1;
#endif
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
` (2 preceding siblings ...)
2013-11-18 21:58 ` [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest Elena Ufimtseva
@ 2013-11-18 21:58 ` Elena Ufimtseva
2013-11-19 11:54 ` David Vrabel
2013-11-19 11:54 ` David Vrabel
2013-11-19 7:18 ` [Xen-devel] [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Dario Faggioli
2013-11-19 7:18 ` Dario Faggioli
5 siblings, 2 replies; 27+ messages in thread
From: Elena Ufimtseva @ 2013-11-18 21:58 UTC (permalink / raw)
To: xen-devel
Cc: konrad.wilk, boris.ostrovsky, david.vrabel, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel, Elena Ufimtseva
Enables numa if vnuma topology hypercall is supported and it is domU.
Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
---
arch/x86/xen/setup.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 68c054f..0aab799 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -20,6 +20,7 @@
#include <asm/numa.h>
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
+#include <asm/xen/vnuma.h>
#include <xen/xen.h>
#include <xen/page.h>
@@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
WARN_ON(xen_set_default_idle());
fiddle_vdso();
#ifdef CONFIG_NUMA
- numa_off = 1;
+ if (!xen_initial_domain() && xen_vnuma_supported())
+ numa_off = 0;
+ else
+ numa_off = 1;
#endif
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-18 21:58 ` Elena Ufimtseva
@ 2013-11-19 11:54 ` David Vrabel
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
2013-11-19 11:54 ` David Vrabel
1 sibling, 2 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 11:54 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: xen-devel, konrad.wilk, boris.ostrovsky, tglx, mingo, hpa, x86,
akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On 18/11/13 21:58, Elena Ufimtseva wrote:
> Enables numa if vnuma topology hypercall is supported and it is domU.
[...]
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -20,6 +20,7 @@
> #include <asm/numa.h>
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> +#include <asm/xen/vnuma.h>
>
> #include <xen/xen.h>
> #include <xen/page.h>
> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> WARN_ON(xen_set_default_idle());
> fiddle_vdso();
> #ifdef CONFIG_NUMA
> - numa_off = 1;
> + if (!xen_initial_domain() && xen_vnuma_supported())
> + numa_off = 0;
> + else
> + numa_off = 1;
> #endif
> }
I think this whole #ifdef CONFIG_NUMA can be removed and hence
xen_vnuma_supported() can be removed as well.
For any PV guest we can call the xen_numa_init() and it will do the
right thing.
For dom0, the hypercall will either: return something sensible (if in
the future Xen sets something up), or it will error.
If Xen does not have vnuma support, the hypercall will error.
In both error cases, the dummy numa node is setup as required.
If you do this, you can fold this change in with the previous patch.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 11:54 ` David Vrabel
@ 2013-11-19 14:16 ` Konrad Rzeszutek Wilk
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 14:16 UTC (permalink / raw)
To: David Vrabel
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> On 18/11/13 21:58, Elena Ufimtseva wrote:
> > Enables numa if vnuma topology hypercall is supported and it is domU.
> [...]
> > --- a/arch/x86/xen/setup.c
> > +++ b/arch/x86/xen/setup.c
> > @@ -20,6 +20,7 @@
> > #include <asm/numa.h>
> > #include <asm/xen/hypervisor.h>
> > #include <asm/xen/hypercall.h>
> > +#include <asm/xen/vnuma.h>
> >
> > #include <xen/xen.h>
> > #include <xen/page.h>
> > @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> > WARN_ON(xen_set_default_idle());
> > fiddle_vdso();
> > #ifdef CONFIG_NUMA
> > - numa_off = 1;
> > + if (!xen_initial_domain() && xen_vnuma_supported())
> > + numa_off = 0;
> > + else
> > + numa_off = 1;
> > #endif
> > }
>
> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> xen_vnuma_supported() can be removed as well.
>
> For any PV guest we can call the xen_numa_init() and it will do the
> right thing.
>
> For dom0, the hypercall will either: return something sensible (if in
> the future Xen sets something up), or it will error.
>
> If Xen does not have vnuma support, the hypercall will error.
>
> In both error cases, the dummy numa node is setup as required.
Incorrect. It will end up calling:
if (!numa_init(amd_numa_init))
which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
as that amd_numa_init is called before the dummy node init.
>
> If you do this, you can fold this change in with the previous patch.
>
> David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 11:54 ` David Vrabel
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
@ 2013-11-19 14:16 ` Konrad Rzeszutek Wilk
2013-11-19 14:35 ` David Vrabel
2013-11-19 14:35 ` David Vrabel
1 sibling, 2 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 14:16 UTC (permalink / raw)
To: David Vrabel
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> On 18/11/13 21:58, Elena Ufimtseva wrote:
> > Enables numa if vnuma topology hypercall is supported and it is domU.
> [...]
> > --- a/arch/x86/xen/setup.c
> > +++ b/arch/x86/xen/setup.c
> > @@ -20,6 +20,7 @@
> > #include <asm/numa.h>
> > #include <asm/xen/hypervisor.h>
> > #include <asm/xen/hypercall.h>
> > +#include <asm/xen/vnuma.h>
> >
> > #include <xen/xen.h>
> > #include <xen/page.h>
> > @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> > WARN_ON(xen_set_default_idle());
> > fiddle_vdso();
> > #ifdef CONFIG_NUMA
> > - numa_off = 1;
> > + if (!xen_initial_domain() && xen_vnuma_supported())
> > + numa_off = 0;
> > + else
> > + numa_off = 1;
> > #endif
> > }
>
> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> xen_vnuma_supported() can be removed as well.
>
> For any PV guest we can call the xen_numa_init() and it will do the
> right thing.
>
> For dom0, the hypercall will either: return something sensible (if in
> the future Xen sets something up), or it will error.
>
> If Xen does not have vnuma support, the hypercall will error.
>
> In both error cases, the dummy numa node is setup as required.
Incorrect. It will end up calling:
if (!numa_init(amd_numa_init))
which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
as that amd_numa_init is called before the dummy node init.
>
> If you do this, you can fold this change in with the previous patch.
>
> David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
@ 2013-11-19 14:35 ` David Vrabel
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
2013-11-19 14:35 ` David Vrabel
1 sibling, 2 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 14:35 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
>> On 18/11/13 21:58, Elena Ufimtseva wrote:
>>> Enables numa if vnuma topology hypercall is supported and it is domU.
>> [...]
>>> --- a/arch/x86/xen/setup.c
>>> +++ b/arch/x86/xen/setup.c
>>> @@ -20,6 +20,7 @@
>>> #include <asm/numa.h>
>>> #include <asm/xen/hypervisor.h>
>>> #include <asm/xen/hypercall.h>
>>> +#include <asm/xen/vnuma.h>
>>>
>>> #include <xen/xen.h>
>>> #include <xen/page.h>
>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
>>> WARN_ON(xen_set_default_idle());
>>> fiddle_vdso();
>>> #ifdef CONFIG_NUMA
>>> - numa_off = 1;
>>> + if (!xen_initial_domain() && xen_vnuma_supported())
>>> + numa_off = 0;
>>> + else
>>> + numa_off = 1;
>>> #endif
>>> }
>>
>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
>> xen_vnuma_supported() can be removed as well.
>>
>> For any PV guest we can call the xen_numa_init() and it will do the
>> right thing.
>>
>> For dom0, the hypercall will either: return something sensible (if in
>> the future Xen sets something up), or it will error.
>>
>> If Xen does not have vnuma support, the hypercall will error.
>>
>> In both error cases, the dummy numa node is setup as required.
>
> Incorrect. It will end up calling:
>
> if (!numa_init(amd_numa_init))
>
> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> as that amd_numa_init is called before the dummy node init.
No it won't. Any error path after the check for a PV guest will add the
dummy node and return success, skipping any of the hardware-specific setup.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:35 ` David Vrabel
@ 2013-11-19 14:46 ` Konrad Rzeszutek Wilk
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 14:46 UTC (permalink / raw)
To: David Vrabel
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> >> On 18/11/13 21:58, Elena Ufimtseva wrote:
> >>> Enables numa if vnuma topology hypercall is supported and it is domU.
> >> [...]
> >>> --- a/arch/x86/xen/setup.c
> >>> +++ b/arch/x86/xen/setup.c
> >>> @@ -20,6 +20,7 @@
> >>> #include <asm/numa.h>
> >>> #include <asm/xen/hypervisor.h>
> >>> #include <asm/xen/hypercall.h>
> >>> +#include <asm/xen/vnuma.h>
> >>>
> >>> #include <xen/xen.h>
> >>> #include <xen/page.h>
> >>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> >>> WARN_ON(xen_set_default_idle());
> >>> fiddle_vdso();
> >>> #ifdef CONFIG_NUMA
> >>> - numa_off = 1;
> >>> + if (!xen_initial_domain() && xen_vnuma_supported())
> >>> + numa_off = 0;
> >>> + else
> >>> + numa_off = 1;
> >>> #endif
> >>> }
> >>
> >> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> >> xen_vnuma_supported() can be removed as well.
> >>
> >> For any PV guest we can call the xen_numa_init() and it will do the
> >> right thing.
> >>
> >> For dom0, the hypercall will either: return something sensible (if in
> >> the future Xen sets something up), or it will error.
> >>
> >> If Xen does not have vnuma support, the hypercall will error.
> >>
> >> In both error cases, the dummy numa node is setup as required.
> >
> > Incorrect. It will end up calling:
> >
> > if (!numa_init(amd_numa_init))
> >
> > which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> > as that amd_numa_init is called before the dummy node init.
>
> No it won't. Any error path after the check for a PV guest will add the
> dummy node and return success, skipping any of the hardware-specific setup.
Duh! I totally missed 'return' at the end of the check!
However, even with that (so the return), that means
this part won't be called:
649 numa_init(dummy_numa_init);
Which means there won't be any dummy numa setup?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:35 ` David Vrabel
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
@ 2013-11-19 14:46 ` Konrad Rzeszutek Wilk
2013-11-19 14:56 ` David Vrabel
2013-11-19 14:56 ` David Vrabel
1 sibling, 2 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 14:46 UTC (permalink / raw)
To: David Vrabel
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> >> On 18/11/13 21:58, Elena Ufimtseva wrote:
> >>> Enables numa if vnuma topology hypercall is supported and it is domU.
> >> [...]
> >>> --- a/arch/x86/xen/setup.c
> >>> +++ b/arch/x86/xen/setup.c
> >>> @@ -20,6 +20,7 @@
> >>> #include <asm/numa.h>
> >>> #include <asm/xen/hypervisor.h>
> >>> #include <asm/xen/hypercall.h>
> >>> +#include <asm/xen/vnuma.h>
> >>>
> >>> #include <xen/xen.h>
> >>> #include <xen/page.h>
> >>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> >>> WARN_ON(xen_set_default_idle());
> >>> fiddle_vdso();
> >>> #ifdef CONFIG_NUMA
> >>> - numa_off = 1;
> >>> + if (!xen_initial_domain() && xen_vnuma_supported())
> >>> + numa_off = 0;
> >>> + else
> >>> + numa_off = 1;
> >>> #endif
> >>> }
> >>
> >> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> >> xen_vnuma_supported() can be removed as well.
> >>
> >> For any PV guest we can call the xen_numa_init() and it will do the
> >> right thing.
> >>
> >> For dom0, the hypercall will either: return something sensible (if in
> >> the future Xen sets something up), or it will error.
> >>
> >> If Xen does not have vnuma support, the hypercall will error.
> >>
> >> In both error cases, the dummy numa node is setup as required.
> >
> > Incorrect. It will end up calling:
> >
> > if (!numa_init(amd_numa_init))
> >
> > which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> > as that amd_numa_init is called before the dummy node init.
>
> No it won't. Any error path after the check for a PV guest will add the
> dummy node and return success, skipping any of the hardware-specific setup.
Duh! I totally missed 'return' at the end of the check!
However, even with that (so the return), that means
this part won't be called:
649 numa_init(dummy_numa_init);
Which means there won't be any dummy numa setup?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
@ 2013-11-19 14:56 ` David Vrabel
2013-11-19 14:56 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 14:56 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
>> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
>>>> On 18/11/13 21:58, Elena Ufimtseva wrote:
>>>>> Enables numa if vnuma topology hypercall is supported and it is domU.
>>>> [...]
>>>>> --- a/arch/x86/xen/setup.c
>>>>> +++ b/arch/x86/xen/setup.c
>>>>> @@ -20,6 +20,7 @@
>>>>> #include <asm/numa.h>
>>>>> #include <asm/xen/hypervisor.h>
>>>>> #include <asm/xen/hypercall.h>
>>>>> +#include <asm/xen/vnuma.h>
>>>>>
>>>>> #include <xen/xen.h>
>>>>> #include <xen/page.h>
>>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
>>>>> WARN_ON(xen_set_default_idle());
>>>>> fiddle_vdso();
>>>>> #ifdef CONFIG_NUMA
>>>>> - numa_off = 1;
>>>>> + if (!xen_initial_domain() && xen_vnuma_supported())
>>>>> + numa_off = 0;
>>>>> + else
>>>>> + numa_off = 1;
>>>>> #endif
>>>>> }
>>>>
>>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
>>>> xen_vnuma_supported() can be removed as well.
>>>>
>>>> For any PV guest we can call the xen_numa_init() and it will do the
>>>> right thing.
>>>>
>>>> For dom0, the hypercall will either: return something sensible (if in
>>>> the future Xen sets something up), or it will error.
>>>>
>>>> If Xen does not have vnuma support, the hypercall will error.
>>>>
>>>> In both error cases, the dummy numa node is setup as required.
>>>
>>> Incorrect. It will end up calling:
>>>
>>> if (!numa_init(amd_numa_init))
>>>
>>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
>>> as that amd_numa_init is called before the dummy node init.
>>
>> No it won't. Any error path after the check for a PV guest will add the
>> dummy node and return success, skipping any of the hardware-specific setup.
>
> Duh! I totally missed 'return' at the end of the check!
>
> However, even with that (so the return), that means
> this part won't be called:
>
> 649 numa_init(dummy_numa_init);
>
> Which means there won't be any dummy numa setup?
The relevant bits in dummy_numa_init are in the error path of
xen_numa_init().
I do think this approach (using the provided API to setup the single
(dummy) node), is preferable to calling dummy_numa_init().
If I thought the hypervisor ABI was finalized, I'd be happy with this
series as-is -- the remaining issues are superficial.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:46 ` Konrad Rzeszutek Wilk
2013-11-19 14:56 ` David Vrabel
@ 2013-11-19 14:56 ` David Vrabel
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
` (3 more replies)
1 sibling, 4 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 14:56 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
>> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
>>>> On 18/11/13 21:58, Elena Ufimtseva wrote:
>>>>> Enables numa if vnuma topology hypercall is supported and it is domU.
>>>> [...]
>>>>> --- a/arch/x86/xen/setup.c
>>>>> +++ b/arch/x86/xen/setup.c
>>>>> @@ -20,6 +20,7 @@
>>>>> #include <asm/numa.h>
>>>>> #include <asm/xen/hypervisor.h>
>>>>> #include <asm/xen/hypercall.h>
>>>>> +#include <asm/xen/vnuma.h>
>>>>>
>>>>> #include <xen/xen.h>
>>>>> #include <xen/page.h>
>>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
>>>>> WARN_ON(xen_set_default_idle());
>>>>> fiddle_vdso();
>>>>> #ifdef CONFIG_NUMA
>>>>> - numa_off = 1;
>>>>> + if (!xen_initial_domain() && xen_vnuma_supported())
>>>>> + numa_off = 0;
>>>>> + else
>>>>> + numa_off = 1;
>>>>> #endif
>>>>> }
>>>>
>>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
>>>> xen_vnuma_supported() can be removed as well.
>>>>
>>>> For any PV guest we can call the xen_numa_init() and it will do the
>>>> right thing.
>>>>
>>>> For dom0, the hypercall will either: return something sensible (if in
>>>> the future Xen sets something up), or it will error.
>>>>
>>>> If Xen does not have vnuma support, the hypercall will error.
>>>>
>>>> In both error cases, the dummy numa node is setup as required.
>>>
>>> Incorrect. It will end up calling:
>>>
>>> if (!numa_init(amd_numa_init))
>>>
>>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
>>> as that amd_numa_init is called before the dummy node init.
>>
>> No it won't. Any error path after the check for a PV guest will add the
>> dummy node and return success, skipping any of the hardware-specific setup.
>
> Duh! I totally missed 'return' at the end of the check!
>
> However, even with that (so the return), that means
> this part won't be called:
>
> 649 numa_init(dummy_numa_init);
>
> Which means there won't be any dummy numa setup?
The relevant bits in dummy_numa_init are in the error path of
xen_numa_init().
I do think this approach (using the provided API to setup the single
(dummy) node), is preferable to calling dummy_numa_init().
If I thought the hypervisor ABI was finalized, I'd be happy with this
series as-is -- the remaining issues are superficial.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:56 ` David Vrabel
@ 2013-11-19 15:19 ` Konrad Rzeszutek Wilk
2013-11-19 15:55 ` David Vrabel
2013-11-19 15:55 ` David Vrabel
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
` (2 subsequent siblings)
3 siblings, 2 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 15:19 UTC (permalink / raw)
To: David Vrabel
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
> On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
> >> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> >>>> On 18/11/13 21:58, Elena Ufimtseva wrote:
> >>>>> Enables numa if vnuma topology hypercall is supported and it is domU.
> >>>> [...]
> >>>>> --- a/arch/x86/xen/setup.c
> >>>>> +++ b/arch/x86/xen/setup.c
> >>>>> @@ -20,6 +20,7 @@
> >>>>> #include <asm/numa.h>
> >>>>> #include <asm/xen/hypervisor.h>
> >>>>> #include <asm/xen/hypercall.h>
> >>>>> +#include <asm/xen/vnuma.h>
> >>>>>
> >>>>> #include <xen/xen.h>
> >>>>> #include <xen/page.h>
> >>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> >>>>> WARN_ON(xen_set_default_idle());
> >>>>> fiddle_vdso();
> >>>>> #ifdef CONFIG_NUMA
> >>>>> - numa_off = 1;
> >>>>> + if (!xen_initial_domain() && xen_vnuma_supported())
> >>>>> + numa_off = 0;
> >>>>> + else
> >>>>> + numa_off = 1;
> >>>>> #endif
> >>>>> }
> >>>>
> >>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> >>>> xen_vnuma_supported() can be removed as well.
> >>>>
> >>>> For any PV guest we can call the xen_numa_init() and it will do the
> >>>> right thing.
> >>>>
> >>>> For dom0, the hypercall will either: return something sensible (if in
> >>>> the future Xen sets something up), or it will error.
> >>>>
> >>>> If Xen does not have vnuma support, the hypercall will error.
> >>>>
> >>>> In both error cases, the dummy numa node is setup as required.
> >>>
> >>> Incorrect. It will end up calling:
> >>>
> >>> if (!numa_init(amd_numa_init))
> >>>
> >>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> >>> as that amd_numa_init is called before the dummy node init.
> >>
> >> No it won't. Any error path after the check for a PV guest will add the
> >> dummy node and return success, skipping any of the hardware-specific setup.
> >
> > Duh! I totally missed 'return' at the end of the check!
> >
> > However, even with that (so the return), that means
> > this part won't be called:
> >
> > 649 numa_init(dummy_numa_init);
> >
> > Which means there won't be any dummy numa setup?
>
> The relevant bits in dummy_numa_init are in the error path of
> xen_numa_init().
That seems the wrong place to do it. The top layer calls
in each of the numa implementations and then falls back to
the dummy.
Calling from within the implementation on something that is eventually
done on the upper level already is not right.
>
> I do think this approach (using the provided API to setup the single
> (dummy) node), is preferable to calling dummy_numa_init().
Doesn't it do the same thing? And also what about if you the user
provides fakenuma?
>
> If I thought the hypervisor ABI was finalized, I'd be happy with this
> series as-is -- the remaining issues are superficial.
That reads to me as an Ack, but I know you like to have it stated
explicitly - so could you state the proper tag please?
>
> David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
@ 2013-11-19 15:55 ` David Vrabel
2013-11-19 16:20 ` Konrad Rzeszutek Wilk
2013-11-19 16:20 ` Konrad Rzeszutek Wilk
2013-11-19 15:55 ` David Vrabel
1 sibling, 2 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 15:55 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
>> The relevant bits in dummy_numa_init are in the error path of
>> xen_numa_init().
>
> That seems the wrong place to do it. The top layer calls
> in each of the numa implementations and then falls back to
> the dummy.
Think of it as not the dummy, but Xen setting the NUMA configuration up
with only a single node.
The useful bits in dummy_numa_init() are two calls to standard functions
for use by *_numa_init() calls so it just seems easier all round to just
call then directly than add a dependancy on dummy_numa_init().
> Calling from within the implementation on something that is eventually
> done on the upper level already is not right.
>From the point of view of the caller, it does the right thing. NUMA is
setup.
>> I do think this approach (using the provided API to setup the single
>> (dummy) node), is preferable to calling dummy_numa_init().
>
> Doesn't it do the same thing? And also what about if you the user
> provides fakenuma?
I don't know what "fakenuma" is refering to.
>> If I thought the hypervisor ABI was finalized, I'd be happy with this
>> series as-is -- the remaining issues are superficial.
>
> That reads to me as an Ack, but I know you like to have it stated
> explicitly - so could you state the proper tag please?
"If I thought the hypervisor ABI was finalized..." is a pretty big "if"
so I have deliberately /not/ given an ack or a reviewed tag but I've
tried to be clear than I think the Linux side is now good enough (except
for any changes for any updates to the hypervisor ABI).
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 15:55 ` David Vrabel
@ 2013-11-19 16:20 ` Konrad Rzeszutek Wilk
2013-11-19 16:20 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 16:20 UTC (permalink / raw)
To: David Vrabel
Cc: Elena Ufimtseva, xen-devel, boris.ostrovsky, tglx, mingo, hpa,
x86, akpm, tangchen, wency, ian.campbell, stefano.stabellini,
mukesh.rathor, linux-kernel
On Tue, Nov 19, 2013 at 03:55:06PM +0000, David Vrabel wrote:
> On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
> >> The relevant bits in dummy_numa_init are in the error path of
> >> xen_numa_init().
> >
> > That seems the wrong place to do it. The top layer calls
> > in each of the numa implementations and then falls back to
> > the dummy.
>
> Think of it as not the dummy, but Xen setting the NUMA configuration up
> with only a single node.
Or with multiple nodes if numa=fake is used.
>
> The useful bits in dummy_numa_init() are two calls to standard functions
> for use by *_numa_init() calls so it just seems easier all round to just
> call then directly than add a dependancy on dummy_numa_init().
My worry is more of the numa_init not being completly executed.
>
> > Calling from within the implementation on something that is eventually
> > done on the upper level already is not right.
>
> >From the point of view of the caller, it does the right thing. NUMA is
> setup.
>
> >> I do think this approach (using the provided API to setup the single
> >> (dummy) node), is preferable to calling dummy_numa_init().
> >
> > Doesn't it do the same thing? And also what about if you the user
> > provides fakenuma?
>
> I don't know what "fakenuma" is refering to.
numa=fake or see arch/x86/mm/numa_emulation.c
Which gets executed if you end up calling: numa_init(X) and 'X'
returns a zero or positive value.
Which it won't as the first thing it the Xen PV NUMA code does
is to see if the hypercall is supported. If not, it bails out - so we
never get to do the 'numa=fake' if a user wanted to.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 15:55 ` David Vrabel
2013-11-19 16:20 ` Konrad Rzeszutek Wilk
@ 2013-11-19 16:20 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 16:20 UTC (permalink / raw)
To: David Vrabel
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On Tue, Nov 19, 2013 at 03:55:06PM +0000, David Vrabel wrote:
> On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
> >> The relevant bits in dummy_numa_init are in the error path of
> >> xen_numa_init().
> >
> > That seems the wrong place to do it. The top layer calls
> > in each of the numa implementations and then falls back to
> > the dummy.
>
> Think of it as not the dummy, but Xen setting the NUMA configuration up
> with only a single node.
Or with multiple nodes if numa=fake is used.
>
> The useful bits in dummy_numa_init() are two calls to standard functions
> for use by *_numa_init() calls so it just seems easier all round to just
> call then directly than add a dependancy on dummy_numa_init().
My worry is more of the numa_init not being completly executed.
>
> > Calling from within the implementation on something that is eventually
> > done on the upper level already is not right.
>
> >From the point of view of the caller, it does the right thing. NUMA is
> setup.
>
> >> I do think this approach (using the provided API to setup the single
> >> (dummy) node), is preferable to calling dummy_numa_init().
> >
> > Doesn't it do the same thing? And also what about if you the user
> > provides fakenuma?
>
> I don't know what "fakenuma" is refering to.
numa=fake or see arch/x86/mm/numa_emulation.c
Which gets executed if you end up calling: numa_init(X) and 'X'
returns a zero or positive value.
Which it won't as the first thing it the Xen PV NUMA code does
is to see if the hypercall is supported. If not, it bails out - so we
never get to do the 'numa=fake' if a user wanted to.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
2013-11-19 15:55 ` David Vrabel
@ 2013-11-19 15:55 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 15:55 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
>> The relevant bits in dummy_numa_init are in the error path of
>> xen_numa_init().
>
> That seems the wrong place to do it. The top layer calls
> in each of the numa implementations and then falls back to
> the dummy.
Think of it as not the dummy, but Xen setting the NUMA configuration up
with only a single node.
The useful bits in dummy_numa_init() are two calls to standard functions
for use by *_numa_init() calls so it just seems easier all round to just
call then directly than add a dependancy on dummy_numa_init().
> Calling from within the implementation on something that is eventually
> done on the upper level already is not right.
>From the point of view of the caller, it does the right thing. NUMA is
setup.
>> I do think this approach (using the provided API to setup the single
>> (dummy) node), is preferable to calling dummy_numa_init().
>
> Doesn't it do the same thing? And also what about if you the user
> provides fakenuma?
I don't know what "fakenuma" is refering to.
>> If I thought the hypervisor ABI was finalized, I'd be happy with this
>> series as-is -- the remaining issues are superficial.
>
> That reads to me as an Ack, but I know you like to have it stated
> explicitly - so could you state the proper tag please?
"If I thought the hypervisor ABI was finalized..." is a pretty big "if"
so I have deliberately /not/ given an ack or a reviewed tag but I've
tried to be clear than I think the Linux side is now good enough (except
for any changes for any updates to the hypervisor ABI).
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:56 ` David Vrabel
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
@ 2013-11-19 15:19 ` Konrad Rzeszutek Wilk
2013-11-23 16:48 ` Dario Faggioli
2013-11-23 16:48 ` [Xen-devel] " Dario Faggioli
3 siblings, 0 replies; 27+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-11-19 15:19 UTC (permalink / raw)
To: David Vrabel
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:
> On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:
> >> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
> >>>> On 18/11/13 21:58, Elena Ufimtseva wrote:
> >>>>> Enables numa if vnuma topology hypercall is supported and it is domU.
> >>>> [...]
> >>>>> --- a/arch/x86/xen/setup.c
> >>>>> +++ b/arch/x86/xen/setup.c
> >>>>> @@ -20,6 +20,7 @@
> >>>>> #include <asm/numa.h>
> >>>>> #include <asm/xen/hypervisor.h>
> >>>>> #include <asm/xen/hypercall.h>
> >>>>> +#include <asm/xen/vnuma.h>
> >>>>>
> >>>>> #include <xen/xen.h>
> >>>>> #include <xen/page.h>
> >>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> >>>>> WARN_ON(xen_set_default_idle());
> >>>>> fiddle_vdso();
> >>>>> #ifdef CONFIG_NUMA
> >>>>> - numa_off = 1;
> >>>>> + if (!xen_initial_domain() && xen_vnuma_supported())
> >>>>> + numa_off = 0;
> >>>>> + else
> >>>>> + numa_off = 1;
> >>>>> #endif
> >>>>> }
> >>>>
> >>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
> >>>> xen_vnuma_supported() can be removed as well.
> >>>>
> >>>> For any PV guest we can call the xen_numa_init() and it will do the
> >>>> right thing.
> >>>>
> >>>> For dom0, the hypercall will either: return something sensible (if in
> >>>> the future Xen sets something up), or it will error.
> >>>>
> >>>> If Xen does not have vnuma support, the hypercall will error.
> >>>>
> >>>> In both error cases, the dummy numa node is setup as required.
> >>>
> >>> Incorrect. It will end up calling:
> >>>
> >>> if (!numa_init(amd_numa_init))
> >>>
> >>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> >>> as that amd_numa_init is called before the dummy node init.
> >>
> >> No it won't. Any error path after the check for a PV guest will add the
> >> dummy node and return success, skipping any of the hardware-specific setup.
> >
> > Duh! I totally missed 'return' at the end of the check!
> >
> > However, even with that (so the return), that means
> > this part won't be called:
> >
> > 649 numa_init(dummy_numa_init);
> >
> > Which means there won't be any dummy numa setup?
>
> The relevant bits in dummy_numa_init are in the error path of
> xen_numa_init().
That seems the wrong place to do it. The top layer calls
in each of the numa implementations and then falls back to
the dummy.
Calling from within the implementation on something that is eventually
done on the upper level already is not right.
>
> I do think this approach (using the provided API to setup the single
> (dummy) node), is preferable to calling dummy_numa_init().
Doesn't it do the same thing? And also what about if you the user
provides fakenuma?
>
> If I thought the hypervisor ABI was finalized, I'd be happy with this
> series as-is -- the remaining issues are superficial.
That reads to me as an Ack, but I know you like to have it stated
explicitly - so could you state the proper tag please?
>
> David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:56 ` David Vrabel
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
2013-11-19 15:19 ` Konrad Rzeszutek Wilk
@ 2013-11-23 16:48 ` Dario Faggioli
2013-11-23 16:48 ` [Xen-devel] " Dario Faggioli
3 siblings, 0 replies; 27+ messages in thread
From: Dario Faggioli @ 2013-11-23 16:48 UTC (permalink / raw)
To: David Vrabel
Cc: ian.campbell, wency, stefano.stabellini, x86, linux-kernel,
mingo, tglx, Elena Ufimtseva, hpa, xen-devel, akpm, tangchen,
boris.ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 1330 bytes --]
On mar, 2013-11-19 at 14:56 +0000, David Vrabel wrote:
> The relevant bits in dummy_numa_init are in the error path of
> xen_numa_init().
>
> I do think this approach (using the provided API to setup the single
> (dummy) node), is preferable to calling dummy_numa_init().
>
> If I thought the hypervisor ABI was finalized, I'd be happy with this
> series as-is -- the remaining issues are superficial.
>
So, David, in this regard, what do you think about these (and the
following messages in the thread?
http://bugs.xenproject.org/xen/mid/%3C528B7D350200007800104840@nat28.tlf.novell.com%3E
http://bugs.xenproject.org/xen/mid/%3C1384871712.19880.90.camel@Abyss%3E
http://bugs.xenproject.org/xen/mid/%3C528B885702000078001048CF@nat28.tlf.novell.com%3E
http://bugs.xenproject.org/xen/mid/%3C1384875772.15360.6.camel@Solace%3E
http://bugs.xenproject.org/xen/mid/%3C528B97C502000078001049AE@nat28.tlf.novell.com%3E
I think Jan has a point about how we are right now retrieving and
treating the number of vnodes...
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:56 ` David Vrabel
` (2 preceding siblings ...)
2013-11-23 16:48 ` Dario Faggioli
@ 2013-11-23 16:48 ` Dario Faggioli
3 siblings, 0 replies; 27+ messages in thread
From: Dario Faggioli @ 2013-11-23 16:48 UTC (permalink / raw)
To: David Vrabel
Cc: Konrad Rzeszutek Wilk, akpm, wency, stefano.stabellini, x86,
linux-kernel, tangchen, mingo, Elena Ufimtseva, hpa, xen-devel,
boris.ostrovsky, tglx, ian.campbell
[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]
On mar, 2013-11-19 at 14:56 +0000, David Vrabel wrote:
> The relevant bits in dummy_numa_init are in the error path of
> xen_numa_init().
>
> I do think this approach (using the provided API to setup the single
> (dummy) node), is preferable to calling dummy_numa_init().
>
> If I thought the hypervisor ABI was finalized, I'd be happy with this
> series as-is -- the remaining issues are superficial.
>
So, David, in this regard, what do you think about these (and the
following messages in the thread?
http://bugs.xenproject.org/xen/mid/%3C528B7D350200007800104840@nat28.tlf.novell.com%3E
http://bugs.xenproject.org/xen/mid/%3C1384871712.19880.90.camel@Abyss%3E
http://bugs.xenproject.org/xen/mid/%3C528B885702000078001048CF@nat28.tlf.novell.com%3E
http://bugs.xenproject.org/xen/mid/%3C1384875772.15360.6.camel@Solace%3E
http://bugs.xenproject.org/xen/mid/%3C528B97C502000078001049AE@nat28.tlf.novell.com%3E
I think Jan has a point about how we are right now retrieving and
treating the number of vnodes...
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-19 14:16 ` Konrad Rzeszutek Wilk
2013-11-19 14:35 ` David Vrabel
@ 2013-11-19 14:35 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 14:35 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: akpm, wency, stefano.stabellini, x86, linux-kernel, tangchen,
mingo, Elena Ufimtseva, hpa, xen-devel, boris.ostrovsky, tglx,
ian.campbell
On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:
>> On 18/11/13 21:58, Elena Ufimtseva wrote:
>>> Enables numa if vnuma topology hypercall is supported and it is domU.
>> [...]
>>> --- a/arch/x86/xen/setup.c
>>> +++ b/arch/x86/xen/setup.c
>>> @@ -20,6 +20,7 @@
>>> #include <asm/numa.h>
>>> #include <asm/xen/hypervisor.h>
>>> #include <asm/xen/hypercall.h>
>>> +#include <asm/xen/vnuma.h>
>>>
>>> #include <xen/xen.h>
>>> #include <xen/page.h>
>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
>>> WARN_ON(xen_set_default_idle());
>>> fiddle_vdso();
>>> #ifdef CONFIG_NUMA
>>> - numa_off = 1;
>>> + if (!xen_initial_domain() && xen_vnuma_supported())
>>> + numa_off = 0;
>>> + else
>>> + numa_off = 1;
>>> #endif
>>> }
>>
>> I think this whole #ifdef CONFIG_NUMA can be removed and hence
>> xen_vnuma_supported() can be removed as well.
>>
>> For any PV guest we can call the xen_numa_init() and it will do the
>> right thing.
>>
>> For dom0, the hypercall will either: return something sensible (if in
>> the future Xen sets something up), or it will error.
>>
>> If Xen does not have vnuma support, the hypercall will error.
>>
>> In both error cases, the dummy numa node is setup as required.
>
> Incorrect. It will end up calling:
>
> if (!numa_init(amd_numa_init))
>
> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.")
> as that amd_numa_init is called before the dummy node init.
No it won't. Any error path after the check for a PV guest will add the
dummy node and return success, skipping any of the hardware-specific setup.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
2013-11-18 21:58 ` Elena Ufimtseva
2013-11-19 11:54 ` David Vrabel
@ 2013-11-19 11:54 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2013-11-19 11:54 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: akpm, wency, x86, linux-kernel, tangchen, mingo, hpa, xen-devel,
boris.ostrovsky, tglx, stefano.stabellini, ian.campbell
On 18/11/13 21:58, Elena Ufimtseva wrote:
> Enables numa if vnuma topology hypercall is supported and it is domU.
[...]
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -20,6 +20,7 @@
> #include <asm/numa.h>
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> +#include <asm/xen/vnuma.h>
>
> #include <xen/xen.h>
> #include <xen/page.h>
> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void)
> WARN_ON(xen_set_default_idle());
> fiddle_vdso();
> #ifdef CONFIG_NUMA
> - numa_off = 1;
> + if (!xen_initial_domain() && xen_vnuma_supported())
> + numa_off = 0;
> + else
> + numa_off = 1;
> #endif
> }
I think this whole #ifdef CONFIG_NUMA can be removed and hence
xen_vnuma_supported() can be removed as well.
For any PV guest we can call the xen_numa_init() and it will do the
right thing.
For dom0, the hypercall will either: return something sensible (if in
the future Xen sets something up), or it will error.
If Xen does not have vnuma support, the hypercall will error.
In both error cases, the dummy numa node is setup as required.
If you do this, you can fold this change in with the previous patch.
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
` (3 preceding siblings ...)
2013-11-18 21:58 ` Elena Ufimtseva
@ 2013-11-19 7:18 ` Dario Faggioli
2013-11-19 7:18 ` Dario Faggioli
5 siblings, 0 replies; 27+ messages in thread
From: Dario Faggioli @ 2013-11-19 7:18 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: xen-devel, akpm, wency, x86, linux-kernel, tangchen, mingo,
david.vrabel, hpa, boris.ostrovsky, tglx, stefano.stabellini,
ian.campbell
[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]
On lun, 2013-11-18 at 16:58 -0500, Elena Ufimtseva wrote:
> Xen vnuma introduction.
>
> The patchset introduces vnuma to paravirtualized Xen guests
> runnning as domU.
> Xen subop hypercall is used to retreive vnuma topology information.
> Bases on the retreived topology from Xen, NUMA number of nodes,
> memory ranges, distance table and cpumask is being set.
> If initialization is incorrect, sets 'dummy' node and unsets
> nodemask. vNUMA topology is constructed by Xen toolstack.
>
> Example of vnuma enabled pv domain dmesg:
>
Mmm... So, why the resend? When you do things like this, you usually
tell the reason, if only, to let people know what happened and which
series they should actually look at and review. :-)
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest
2013-11-18 21:58 [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Elena Ufimtseva
` (4 preceding siblings ...)
2013-11-19 7:18 ` [Xen-devel] [PATCH RESEND v2 0/2] xen: vnuma introduction for pv guest Dario Faggioli
@ 2013-11-19 7:18 ` Dario Faggioli
5 siblings, 0 replies; 27+ messages in thread
From: Dario Faggioli @ 2013-11-19 7:18 UTC (permalink / raw)
To: Elena Ufimtseva
Cc: ian.campbell, wency, stefano.stabellini, x86, linux-kernel,
tangchen, mingo, tglx, david.vrabel, hpa, xen-devel, akpm,
boris.ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 1025 bytes --]
On lun, 2013-11-18 at 16:58 -0500, Elena Ufimtseva wrote:
> Xen vnuma introduction.
>
> The patchset introduces vnuma to paravirtualized Xen guests
> runnning as domU.
> Xen subop hypercall is used to retreive vnuma topology information.
> Bases on the retreived topology from Xen, NUMA number of nodes,
> memory ranges, distance table and cpumask is being set.
> If initialization is incorrect, sets 'dummy' node and unsets
> nodemask. vNUMA topology is constructed by Xen toolstack.
>
> Example of vnuma enabled pv domain dmesg:
>
Mmm... So, why the resend? When you do things like this, you usually
tell the reason, if only, to let people know what happened and which
series they should actually look at and review. :-)
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread