* [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa()
@ 2017-01-18 14:48 Igor Mammedov
2017-01-18 15:19 ` Eduardo Habkost
0 siblings, 1 reply; 4+ messages in thread
From: Igor Mammedov @ 2017-01-18 14:48 UTC (permalink / raw)
To: qemu-devel
Cc: Dou Liyang, fanc.fnst, caoj.fnst, stefanha, izumi.taku, vilanova,
ehabkost, peter.maydell, Andrew Jones, David Gibson, Thomas Huth
Move vcpu's assocciated numa_node field out of generic CPUState
into inherited classes that actually care about cpu<->numa mapping
and make monitor 'info numa' get vcpu's assocciated node id via
node-id property.
It allows to drop implicit node id intialization in
numa_post_machine_init() and would allow switching to mapping
defined by target's CpuInstanceProperties instead of global
numa_info[i].node_cpu bitmaps.
As side effect it fixes 'info numa' displaying wrong mapping
for CPUs specified with -device/device_add.
Before patch following CLI would produce:
QEMU -smp 1,sockets=3,maxcpus=3 \
-device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
-numa node,nodeid=0,cpus=0 \
-numa node,nodeid=1,cpus=1 \
-numa node,nodeid=2,cpus=2
(qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
(qemu) info numa
3 nodes
node 0 cpus: 0 1 2
node 0 size: 40 MB
node 1 cpus:
node 1 size: 40 MB
node 2 cpus:
node 2 size: 48 MB
after patch all CPUs are on nodes they are supposed to be:
(qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
(qemu) info numa
3 nodes
node 0 cpus: 0
node 0 size: 40 MB
node 1 cpus: 1
node 1 size: 40 MB
node 2 cpus: 2
node 2 size: 48 MB
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Dou Liyang <douly.fnst@cn.fujitsu.com>
CC: fanc.fnst@cn.fujitsu.com
CC: caoj.fnst@cn.fujitsu.com
CC: stefanha@redhat.com
CC: izumi.taku@jp.fujitsu.com
CC: vilanova@ac.upc.edu
CC: ehabkost@redhat.com
CC: peter.maydell@linaro.org
CC: Andrew Jones <drjones@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Thomas Huth <thuth@redhat.com>
---
include/qom/cpu.h | 2 --
include/sysemu/numa.h | 1 -
target/arm/cpu.h | 2 ++
target/i386/cpu.h | 1 +
target/ppc/cpu.h | 2 ++
hw/arm/virt.c | 12 ++++++++----
hw/i386/pc.c | 5 +++++
hw/ppc/spapr.c | 2 +-
hw/ppc/spapr_cpu_core.c | 2 +-
monitor.c | 7 +++++--
numa.c | 15 ---------------
target/arm/cpu.c | 1 +
target/i386/cpu.c | 1 +
target/ppc/translate_init.c | 1 +
vl.c | 2 --
15 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3f79a8e..ae637a9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -257,7 +257,6 @@ struct qemu_work_item;
* @cpu_index: CPU index (informative).
* @nr_cores: Number of cores within this CPU package.
* @nr_threads: Number of threads within this CPU.
- * @numa_node: NUMA node this CPU is belonging to.
* @host_tid: Host thread ID.
* @running: #true if CPU is currently running (lockless).
* @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
@@ -306,7 +305,6 @@ struct CPUState {
int nr_cores;
int nr_threads;
- int numa_node;
struct QemuThread *thread;
#ifdef _WIN32
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 8f09dcf..b8015a5 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -25,7 +25,6 @@ typedef struct node_info {
extern NodeInfo numa_info[MAX_NODES];
void parse_numa_opts(MachineClass *mc);
-void numa_post_machine_init(void);
void query_numa_node_mem(uint64_t node_mem[]);
extern QemuOptsList qemu_numa_opts;
void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7bd16ee..ef263f1 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -662,6 +662,8 @@ struct ARMCPU {
ARMELChangeHook *el_change_hook;
void *el_change_hook_opaque;
+
+ int32_t numa_nid;
};
static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6c1902b..e43dcc2 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1264,6 +1264,7 @@ struct X86CPU {
int32_t socket_id;
int32_t core_id;
int32_t thread_id;
+ int32_t numa_nid;
};
static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2a50c43..2d12ad5 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1154,6 +1154,7 @@ do { \
* @cpu_dt_id: CPU index used in the device tree. KVM uses this index too
* @max_compat: Maximal supported logical PVR from the command line
* @cpu_version: Current logical PVR, zero if in "raw" mode
+ * @numa_nid: Numa node id the CPU belongs to
*
* A PowerPC CPU.
*/
@@ -1166,6 +1167,7 @@ struct PowerPCCPU {
int cpu_dt_id;
uint32_t max_compat;
uint32_t cpu_version;
+ int32_t numa_nid;
/* Fields related to migration compatibility hacks */
bool pre_2_8_migration;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7a03f84..b86b5fd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -329,7 +329,6 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
{
int cpu;
int addr_cells = 1;
- unsigned int i;
/*
* From Documentation/devicetree/bindings/arm/cpus.txt
@@ -379,9 +378,9 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
armcpu->mp_affinity);
}
- i = numa_get_node_for_cpu(cpu);
- if (i < nb_numa_nodes) {
- qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id", i);
+ if (armcpu->numa_nid < nb_numa_nodes) {
+ qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id",
+ armcpu->numa_nid);
}
g_free(nodename);
@@ -1333,6 +1332,11 @@ static void machvirt_init(MachineState *machine)
"secure-memory", &error_abort);
}
+ if (nb_numa_nodes) {
+ object_property_set_int(cpuobj, numa_get_node_for_cpu(n),
+ "node-id", NULL);
+ }
+
object_property_set_bool(cpuobj, true, "realized", NULL);
}
fdt_add_timer_nodes(vms);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f721fde..9d2b265 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
cs = CPU(cpu);
cs->cpu_index = idx;
+
+ idx = numa_get_node_for_cpu(cs->cpu_index);
+ if (idx < nb_numa_nodes) {
+ cpu->numa_nid = idx;
+ }
}
static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 208ef7b..efcd925 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -182,7 +182,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs)
cpu_to_be32(0x0),
cpu_to_be32(0x0),
cpu_to_be32(0x0),
- cpu_to_be32(cs->numa_node),
+ cpu_to_be32(cpu->numa_nid),
cpu_to_be32(index)};
/* Advertise NUMA via ibm,associativity */
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c18632b..7f6661b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -71,7 +71,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
/* Set NUMA node for the added CPUs */
i = numa_get_node_for_cpu(cs->cpu_index);
if (i < nb_numa_nodes) {
- cs->numa_node = i;
+ cpu->numa_nid = i;
}
xics_cpu_setup(spapr->xics, cpu);
diff --git a/monitor.c b/monitor.c
index 0841d43..8856d5b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1544,9 +1544,12 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
for (i = 0; i < nb_numa_nodes; i++) {
monitor_printf(mon, "node %d cpus:", i);
CPU_FOREACH(cpu) {
- if (cpu->numa_node == i) {
- monitor_printf(mon, " %d", cpu->cpu_index);
+ Error *err = NULL;
+ int64_t nid = object_property_get_int(OBJECT(cpu), "node-id", &err);
+ if (nid == i && !err) {
+ monitor_printf(mon, " %d", cpu->cpu_index);
}
+ error_free(err);
}
monitor_printf(mon, "\n");
monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
diff --git a/numa.c b/numa.c
index 379bc8a..5f68497 100644
--- a/numa.c
+++ b/numa.c
@@ -394,21 +394,6 @@ void parse_numa_opts(MachineClass *mc)
}
}
-void numa_post_machine_init(void)
-{
- CPUState *cpu;
- int i;
-
- CPU_FOREACH(cpu) {
- for (i = 0; i < nb_numa_nodes; i++) {
- assert(cpu->cpu_index < max_cpus);
- if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
- cpu->numa_node = i;
- }
- }
- }
-}
-
static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
const char *name,
uint64_t ram_size)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9104611..8caf853 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1515,6 +1515,7 @@ static Property arm_cpu_properties[] = {
DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
mp_affinity, ARM64_AFFINITY_INVALID),
+ DEFINE_PROP_INT32("node-id", ARMCPU, numa_nid, 0),
DEFINE_PROP_END_OF_LIST()
};
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index aba11ae..85c52f1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3649,6 +3649,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
#endif
+ DEFINE_PROP_INT32("node-id", X86CPU, numa_nid, 0),
DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
{ .name = "hv-spinlocks", .info = &qdev_prop_spinlocks },
DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index e6a835c..64bd7be 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -10520,6 +10520,7 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
static Property ppc_cpu_properties[] = {
DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
+ DEFINE_PROP_INT32("node-id", PowerPCCPU, numa_nid, 0),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/vl.c b/vl.c
index c643d3f..afe40ce 100644
--- a/vl.c
+++ b/vl.c
@@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
cpu_synchronize_all_post_init();
- numa_post_machine_init();
-
if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
exit(1);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa()
2017-01-18 14:48 [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa() Igor Mammedov
@ 2017-01-18 15:19 ` Eduardo Habkost
2017-01-18 17:08 ` Igor Mammedov
0 siblings, 1 reply; 4+ messages in thread
From: Eduardo Habkost @ 2017-01-18 15:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Dou Liyang, fanc.fnst, caoj.fnst, stefanha,
izumi.taku, vilanova, peter.maydell, Andrew Jones, David Gibson,
Thomas Huth
On Wed, Jan 18, 2017 at 03:48:45PM +0100, Igor Mammedov wrote:
> Move vcpu's assocciated numa_node field out of generic CPUState
> into inherited classes that actually care about cpu<->numa mapping
> and make monitor 'info numa' get vcpu's assocciated node id via
> node-id property.
> It allows to drop implicit node id intialization in
> numa_post_machine_init() and would allow switching to mapping
> defined by target's CpuInstanceProperties instead of global
> numa_info[i].node_cpu bitmaps.
I agree to allow the node-id assignment logic to be defined by
arch-specific code, but:
Considering that we still have the generic CPU<->node mapping in
numa_info[].node_cpu, I don't see the benefit of moving the
numa_info[].node_cpu => node-id translation from common generic
code to duplicated arch-specific code.
What about adding this to cpu_generic_realize():
node = numa_get_node_for_cpu(cpu)
if (node >= 0) {
int cur_node = object_property_find(cpu, "node-id") ?
object_property_get_int(cpu, "node-id") : -1;
if (cur_node >= 0 && cur_node != node) {
error_setg(&err, "Conflict between -numa option and CPU node-id");
goto out;
}
object_property_set_int(cpu, "node-id", node, &err);
}
This way, the (legacy?) numa_info.node_cpu table will still work
automatically, but we can implement arch-specific validation of
the property if we want to, and architectures that don't support
NUMA would be able to report an error in case the user tries to
configure NUMA.
>
> As side effect it fixes 'info numa' displaying wrong mapping
> for CPUs specified with -device/device_add.
> Before patch following CLI would produce:
> QEMU -smp 1,sockets=3,maxcpus=3 \
> -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
> -numa node,nodeid=0,cpus=0 \
> -numa node,nodeid=1,cpus=1 \
> -numa node,nodeid=2,cpus=2
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0 1 2
> node 0 size: 40 MB
> node 1 cpus:
> node 1 size: 40 MB
> node 2 cpus:
> node 2 size: 48 MB
>
> after patch all CPUs are on nodes they are supposed to be:
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0
> node 0 size: 40 MB
> node 1 cpus: 1
> node 1 size: 40 MB
> node 2 cpus: 2
> node 2 size: 48 MB
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Dou Liyang <douly.fnst@cn.fujitsu.com>
> CC: fanc.fnst@cn.fujitsu.com
> CC: caoj.fnst@cn.fujitsu.com
> CC: stefanha@redhat.com
> CC: izumi.taku@jp.fujitsu.com
> CC: vilanova@ac.upc.edu
> CC: ehabkost@redhat.com
> CC: peter.maydell@linaro.org
> CC: Andrew Jones <drjones@redhat.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Thomas Huth <thuth@redhat.com>
> ---
> include/qom/cpu.h | 2 --
> include/sysemu/numa.h | 1 -
> target/arm/cpu.h | 2 ++
> target/i386/cpu.h | 1 +
> target/ppc/cpu.h | 2 ++
> hw/arm/virt.c | 12 ++++++++----
> hw/i386/pc.c | 5 +++++
> hw/ppc/spapr.c | 2 +-
> hw/ppc/spapr_cpu_core.c | 2 +-
> monitor.c | 7 +++++--
> numa.c | 15 ---------------
> target/arm/cpu.c | 1 +
> target/i386/cpu.c | 1 +
> target/ppc/translate_init.c | 1 +
> vl.c | 2 --
> 15 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 3f79a8e..ae637a9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -257,7 +257,6 @@ struct qemu_work_item;
> * @cpu_index: CPU index (informative).
> * @nr_cores: Number of cores within this CPU package.
> * @nr_threads: Number of threads within this CPU.
> - * @numa_node: NUMA node this CPU is belonging to.
> * @host_tid: Host thread ID.
> * @running: #true if CPU is currently running (lockless).
> * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
> @@ -306,7 +305,6 @@ struct CPUState {
>
> int nr_cores;
> int nr_threads;
> - int numa_node;
>
> struct QemuThread *thread;
> #ifdef _WIN32
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 8f09dcf..b8015a5 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -25,7 +25,6 @@ typedef struct node_info {
>
> extern NodeInfo numa_info[MAX_NODES];
> void parse_numa_opts(MachineClass *mc);
> -void numa_post_machine_init(void);
> void query_numa_node_mem(uint64_t node_mem[]);
> extern QemuOptsList qemu_numa_opts;
> void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 7bd16ee..ef263f1 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -662,6 +662,8 @@ struct ARMCPU {
>
> ARMELChangeHook *el_change_hook;
> void *el_change_hook_opaque;
> +
> + int32_t numa_nid;
> };
>
> static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 6c1902b..e43dcc2 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1264,6 +1264,7 @@ struct X86CPU {
> int32_t socket_id;
> int32_t core_id;
> int32_t thread_id;
> + int32_t numa_nid;
> };
>
> static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2a50c43..2d12ad5 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1154,6 +1154,7 @@ do { \
> * @cpu_dt_id: CPU index used in the device tree. KVM uses this index too
> * @max_compat: Maximal supported logical PVR from the command line
> * @cpu_version: Current logical PVR, zero if in "raw" mode
> + * @numa_nid: Numa node id the CPU belongs to
> *
> * A PowerPC CPU.
> */
> @@ -1166,6 +1167,7 @@ struct PowerPCCPU {
> int cpu_dt_id;
> uint32_t max_compat;
> uint32_t cpu_version;
> + int32_t numa_nid;
>
> /* Fields related to migration compatibility hacks */
> bool pre_2_8_migration;
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7a03f84..b86b5fd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -329,7 +329,6 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> {
> int cpu;
> int addr_cells = 1;
> - unsigned int i;
>
> /*
> * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -379,9 +378,9 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> armcpu->mp_affinity);
> }
>
> - i = numa_get_node_for_cpu(cpu);
> - if (i < nb_numa_nodes) {
> - qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id", i);
> + if (armcpu->numa_nid < nb_numa_nodes) {
> + qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id",
> + armcpu->numa_nid);
> }
>
> g_free(nodename);
> @@ -1333,6 +1332,11 @@ static void machvirt_init(MachineState *machine)
> "secure-memory", &error_abort);
> }
>
> + if (nb_numa_nodes) {
> + object_property_set_int(cpuobj, numa_get_node_for_cpu(n),
> + "node-id", NULL);
> + }
> +
> object_property_set_bool(cpuobj, true, "realized", NULL);
> }
> fdt_add_timer_nodes(vms);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f721fde..9d2b265 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>
> cs = CPU(cpu);
> cs->cpu_index = idx;
> +
> + idx = numa_get_node_for_cpu(cs->cpu_index);
> + if (idx < nb_numa_nodes) {
> + cpu->numa_nid = idx;
> + }
> }
>
> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 208ef7b..efcd925 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -182,7 +182,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs)
> cpu_to_be32(0x0),
> cpu_to_be32(0x0),
> cpu_to_be32(0x0),
> - cpu_to_be32(cs->numa_node),
> + cpu_to_be32(cpu->numa_nid),
> cpu_to_be32(index)};
>
> /* Advertise NUMA via ibm,associativity */
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c18632b..7f6661b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -71,7 +71,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
> /* Set NUMA node for the added CPUs */
> i = numa_get_node_for_cpu(cs->cpu_index);
> if (i < nb_numa_nodes) {
> - cs->numa_node = i;
> + cpu->numa_nid = i;
> }
>
> xics_cpu_setup(spapr->xics, cpu);
> diff --git a/monitor.c b/monitor.c
> index 0841d43..8856d5b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1544,9 +1544,12 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
> for (i = 0; i < nb_numa_nodes; i++) {
> monitor_printf(mon, "node %d cpus:", i);
> CPU_FOREACH(cpu) {
> - if (cpu->numa_node == i) {
> - monitor_printf(mon, " %d", cpu->cpu_index);
> + Error *err = NULL;
> + int64_t nid = object_property_get_int(OBJECT(cpu), "node-id", &err);
> + if (nid == i && !err) {
> + monitor_printf(mon, " %d", cpu->cpu_index);
> }
> + error_free(err);
> }
> monitor_printf(mon, "\n");
> monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
> diff --git a/numa.c b/numa.c
> index 379bc8a..5f68497 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -394,21 +394,6 @@ void parse_numa_opts(MachineClass *mc)
> }
> }
>
> -void numa_post_machine_init(void)
> -{
> - CPUState *cpu;
> - int i;
> -
> - CPU_FOREACH(cpu) {
> - for (i = 0; i < nb_numa_nodes; i++) {
> - assert(cpu->cpu_index < max_cpus);
> - if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
> - cpu->numa_node = i;
> - }
> - }
> - }
> -}
> -
> static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> const char *name,
> uint64_t ram_size)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9104611..8caf853 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1515,6 +1515,7 @@ static Property arm_cpu_properties[] = {
> DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
> mp_affinity, ARM64_AFFINITY_INVALID),
> + DEFINE_PROP_INT32("node-id", ARMCPU, numa_nid, 0),
> DEFINE_PROP_END_OF_LIST()
> };
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index aba11ae..85c52f1 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3649,6 +3649,7 @@ static Property x86_cpu_properties[] = {
> DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
> #endif
> + DEFINE_PROP_INT32("node-id", X86CPU, numa_nid, 0),
> DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
> { .name = "hv-spinlocks", .info = &qdev_prop_spinlocks },
> DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index e6a835c..64bd7be 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -10520,6 +10520,7 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>
> static Property ppc_cpu_properties[] = {
> DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
> + DEFINE_PROP_INT32("node-id", PowerPCCPU, numa_nid, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/vl.c b/vl.c
> index c643d3f..afe40ce 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
>
> cpu_synchronize_all_post_init();
>
> - numa_post_machine_init();
> -
> if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
> exit(1);
> --
> 2.7.4
>
--
Eduardo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa()
2017-01-18 15:19 ` Eduardo Habkost
@ 2017-01-18 17:08 ` Igor Mammedov
2017-01-18 18:06 ` Eduardo Habkost
0 siblings, 1 reply; 4+ messages in thread
From: Igor Mammedov @ 2017-01-18 17:08 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Dou Liyang, fanc.fnst, caoj.fnst, stefanha,
izumi.taku, vilanova, peter.maydell, Andrew Jones, David Gibson,
Thomas Huth
On Wed, 18 Jan 2017 13:19:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Jan 18, 2017 at 03:48:45PM +0100, Igor Mammedov wrote:
> > Move vcpu's assocciated numa_node field out of generic CPUState
> > into inherited classes that actually care about cpu<->numa mapping
> > and make monitor 'info numa' get vcpu's assocciated node id via
> > node-id property.
> > It allows to drop implicit node id intialization in
> > numa_post_machine_init() and would allow switching to mapping
> > defined by target's CpuInstanceProperties instead of global
> > numa_info[i].node_cpu bitmaps.
>
> I agree to allow the node-id assignment logic to be defined by
> arch-specific code, but:
>
> Considering that we still have the generic CPU<->node mapping in
> numa_info[].node_cpu, I don't see the benefit of moving the
> numa_info[].node_cpu => node-id translation from common generic
> code to duplicated arch-specific code.
it's duplicated for a time being until all targets that use
node_cpu are internally converted to socket/core/thread-id interface.
This patch is a part if re-factoring RFC which I'm about to post.
> What about adding this to cpu_generic_realize():
>
> node = (numa_get_node_for_cpu(cpu)
> if (node >= 0) {
> int cur_node = object_property_find(cpu, "node-id") ?
> object_property_get_int(cpu, "node-id") : -1;
> if (cur_node >= 0 && cur_node != node) {
> error_setg(&err, "Conflict between -numa option and CPU node-id");
> goto out;
> }
> object_property_set_int(cpu, "node-id", node, &err);
> }
What I don't like here is that putting above snippet into
cpu_generic_realize() is just doing the same implicit init
that numa_post_machine_init() has been doing before just
a bit later. It looks like a quick fix for 'info numa' issues.
However it would add stumble block on getting rid of
numa_get_node_for_cpu() { numa_info[].node_cpu } and keeping
node-mapping along with the rest of topology in Machine object
instead of globals.
I'm in process of getting rid of numa_info[].node_cpu/numa_get_node_for_cpu()
altogether. And incrementally done it for PC, so I'd be forced
to reduplicate above snippet from cpu_generic_realize() in concrete
users anyway to do conversion without breaking other users.
[...]
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >
> > cs = CPU(cpu);
> > cs->cpu_index = idx;
> > +
> > + idx = numa_get_node_for_cpu(cs->cpu_index);
> > + if (idx < nb_numa_nodes) {
> > + cpu->numa_nid = idx;
> > + }
this is the only place where I add above mentioned duplication,
which sort of compensated by removed numa_post_machine_init().
And it's here only to keep working+fix 'info numa'.
The other targets currently already do numa_get_node_for_cpu(),
lookup so I'm not changing there anything.
Later(not in this patch) I'm removing numa_get_node_for_cpu() from
pc_cpu_pre_plug() and from PC code altogether and replacing it
with node-id from possible_cpus.
PS:
since travis-ci seems to be broken, I'll post re-factoring
RFC without building there and it includes this patch as well.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa()
2017-01-18 17:08 ` Igor Mammedov
@ 2017-01-18 18:06 ` Eduardo Habkost
0 siblings, 0 replies; 4+ messages in thread
From: Eduardo Habkost @ 2017-01-18 18:06 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Dou Liyang, fanc.fnst, caoj.fnst, stefanha,
izumi.taku, vilanova, peter.maydell, Andrew Jones, David Gibson,
Thomas Huth
On Wed, Jan 18, 2017 at 06:08:24PM +0100, Igor Mammedov wrote:
> On Wed, 18 Jan 2017 13:19:45 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Wed, Jan 18, 2017 at 03:48:45PM +0100, Igor Mammedov wrote:
> > > Move vcpu's assocciated numa_node field out of generic CPUState
> > > into inherited classes that actually care about cpu<->numa mapping
> > > and make monitor 'info numa' get vcpu's assocciated node id via
> > > node-id property.
> > > It allows to drop implicit node id intialization in
> > > numa_post_machine_init() and would allow switching to mapping
> > > defined by target's CpuInstanceProperties instead of global
> > > numa_info[i].node_cpu bitmaps.
> >
> > I agree to allow the node-id assignment logic to be defined by
> > arch-specific code, but:
> >
> > Considering that we still have the generic CPU<->node mapping in
> > numa_info[].node_cpu, I don't see the benefit of moving the
> > numa_info[].node_cpu => node-id translation from common generic
> > code to duplicated arch-specific code.
> it's duplicated for a time being until all targets that use
> node_cpu are internally converted to socket/core/thread-id interface.
> This patch is a part if re-factoring RFC which I'm about to post.
>
>
Maybe the duplication will be OK if it's temporary, and going to
be removed immediately by the same series. But if it is a
standalone patch, I don't want to make the existing duplication
worse.
> > What about adding this to cpu_generic_realize():
> >
> > node = (numa_get_node_for_cpu(cpu)
> > if (node >= 0) {
> > int cur_node = object_property_find(cpu, "node-id") ?
> > object_property_get_int(cpu, "node-id") : -1;
> > if (cur_node >= 0 && cur_node != node) {
> > error_setg(&err, "Conflict between -numa option and CPU node-id");
> > goto out;
> > }
> > object_property_set_int(cpu, "node-id", node, &err);
> > }
> What I don't like here is that putting above snippet into
> cpu_generic_realize() is just doing the same implicit init
> that numa_post_machine_init() has been doing before just
> a bit later. It looks like a quick fix for 'info numa' issues.
Yes, it is. But I still don't see what's the problem with it. If
numa_info[].node_cpu still exists in generic code, I don't see
why not handle it in generic code.
>
> However it would add stumble block on getting rid of
> numa_get_node_for_cpu() { numa_info[].node_cpu } and keeping
> node-mapping along with the rest of topology in Machine object
> instead of globals.
Removing duplication would make it easier: then you'll have only
one user of numa_info[].node_cpu/numa_get_node_for_cpu() to get
rid of, instead of 4.
>
> I'm in process of getting rid of numa_info[].node_cpu/numa_get_node_for_cpu()
> altogether. And incrementally done it for PC, so I'd be forced
> to reduplicate above snippet from cpu_generic_realize() in concrete
> users anyway to do conversion without breaking other users.
If you manage to get rid of numa_info[].node_cpu somehow, then we
could get rid of numa_post_machine_init()/numa_get_node_for_cpu()
at the same time. But I don't see the benefit of duplicating
existing generic code into arch-specific code while keeping the
existing generic numa_info[].node_cpu data structure untouched.
>
> [...]
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > >
> > > cs = CPU(cpu);
> > > cs->cpu_index = idx;
> > > +
> > > + idx = numa_get_node_for_cpu(cs->cpu_index);
> > > + if (idx < nb_numa_nodes) {
> > > + cpu->numa_nid = idx;
> > > + }
> this is the only place where I add above mentioned duplication,
> which sort of compensated by removed numa_post_machine_init().
Is it? I see the same logic duplicated in 3 places (see below).
Not all cases were added by you, but that doesn't mean it is OK
to make it worse.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f721fde..9d2b265 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
cs = CPU(cpu);
cs->cpu_index = idx;
+
+ idx = numa_get_node_for_cpu(cs->cpu_index);
+ if (idx < nb_numa_nodes) {
+ cpu->numa_nid = idx;
+ }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c18632b..7f6661b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -71,7 +71,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
/* Set NUMA node for the added CPUs */
i = numa_get_node_for_cpu(cs->cpu_index);
if (i < nb_numa_nodes) {
- cs->numa_node = i;
+ cpu->numa_nid = i;
}
xics_cpu_setup(spapr->xics, cpu);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7a03f84..b86b5fd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1333,6 +1332,11 @@ static void machvirt_init(MachineState *machine)
"secure-memory", &error_abort);
}
+ if (nb_numa_nodes) {
+ object_property_set_int(cpuobj, numa_get_node_for_cpu(n),
+ "node-id", NULL);
+ }
+
object_property_set_bool(cpuobj, true, "realized", NULL);
}
fdt_add_timer_nodes(vms);
> And it's here only to keep working+fix 'info numa'.
> The other targets currently already do numa_get_node_for_cpu(),
> lookup so I'm not changing there anything.
> Later(not in this patch) I'm removing numa_get_node_for_cpu() from
> pc_cpu_pre_plug() and from PC code altogether and replacing it
> with node-id from possible_cpus.
I will look at the whole series to re-evaluate this patch, then.
>
> PS:
> since travis-ci seems to be broken, I'll post re-factoring
> RFC without building there and it includes this patch as well.
No problem to me, if it's a RFC.
--
Eduardo
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-18 18:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-18 14:48 [Qemu-devel] [PATCH] numa: access CPU's node id via property in hmp_info_numa() Igor Mammedov
2017-01-18 15:19 ` Eduardo Habkost
2017-01-18 17:08 ` Igor Mammedov
2017-01-18 18:06 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).