[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 1/4] numa, spapr: add thread-id in the possible_cp
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [RFC 1/4] numa, spapr: add thread-id in the possible_cpus list |
Date: |
Wed, 13 Feb 2019 09:42:59 +0100 |
On Wed, 13 Feb 2019 12:25:45 +1100
David Gibson <address@hidden> wrote:
> On Tue, Feb 12, 2019 at 10:48:24PM +0100, Laurent Vivier wrote:
> > spapr_possible_cpu_arch_ids() counts only cores, and so
> > the number of available CPUs is the number of vCPU divided
> > by smp_threads.
> >
> > ... -smp 4,maxcpus=8,cores=2,threads=2,sockets=2 -numa node,cpus=0,cpus=1 \
> > -numa node,cpus=3,cpus=4 \
> > -numa node -numa node
> >
> > This generates (info hotpluggable-cpus)
> >
> > node-id: 0 core-id: 0 thread-id: 0 [thread-id: 1]
> > node-id: 0 core-id: 6 thread-id: 0 [thread-id: 1]
> > node-id: 1 core-id: 2 thread-id: 0 [thread-id: 1]
> > node-id: 1 core-id: 4 thread-id: 0 [thread-id: 1]
> >
> > And this command line generates the following error:
> >
> > CPU(s) not present in any NUMA nodes: CPU 3 [core-id: 6]
> >
> > That is wrong because CPU 3 [core-id: 6] is assigned to node-id 0
> > Moreover "cpus=4" is not valid, because it means core-id 8 but
> > maxcpus is 8.
> >
> > With this patch we have now:
> >
> > node-id: 0 core-id: 0 thread-id: 0
> > node-id: 0 core-id: 0 thread-id: 1
> > node-id: 0 core-id: 1 thread-id: 0
> > node-id: 1 core-id: 1 thread-id: 1
> > node-id: 0 core-id: 2 thread-id: 1
> > node-id: 1 core-id: 2 thread-id: 0
> > node-id: 0 core-id: 3 thread-id: 1
> > node-id: 0 core-id: 3 thread-id: 0
>
> I'm afraid this is not the right solution. The point of the
> hotpluggable cpus table is that it has exactly one entry for each
> hotpluggable unit. For PAPR that's a core, not a thread.
>
> So, the problem is with how the NUMA configuration code is
> interpreting possible-cpus, not how the machine is building the table.
I'd suggest to deprecate/remove 'cpus' suboption in -numa.
One should use '-numa cpu' instead, which is written with
possible_cpus in mind.
>
> > CPUs 0 (core-id: 0 thread-id: 0) and 1 (core-id: 0 thread-id: 1) are
> > correctly assigned to node-id 0, CPUs 3 (core-id: 1 thread-id: 1) and
> > 4 (core-id: 2 thread-id: 0) are correctly assigned to node-id 1.
> > All other CPUs are assigned to node-id 0 by default.
> >
> > And the error message is also correct:
> >
> > CPU(s) not present in any NUMA nodes: CPU 2 [core-id: 1, thread-id: 0], \
> > CPU 5 [core-id: 2, thread-id: 1], \
> > CPU 6 [core-id: 3, thread-id: 0], \
> > CPU 7 [core-id: 3, thread-id: 1]
> >
> > Fixes: ec78f8114bc4 ("numa: use possible_cpus for not mapped CPUs check")
> > Cc: address@hidden
> >
> > Before commit ec78f8114bc4, output was correct:
> >
> > CPU(s) not present in any NUMA nodes: 2 5 6 7
> >
> > Signed-off-by: Laurent Vivier <address@hidden>
> > ---
> > hw/ppc/spapr.c | 33 +++++++++++++--------------------
> > 1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 332cba89d425..7196ba09da34 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2404,15 +2404,13 @@ static void spapr_validate_node_memory(MachineState
> > *machine, Error **errp)
> > /* find cpu slot in machine->possible_cpus by core_id */
> > static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int
> > *idx)
> > {
> > - int index = id / smp_threads;
> > -
> > - if (index >= ms->possible_cpus->len) {
> > + if (id >= ms->possible_cpus->len) {
> > return NULL;
> > }
> > if (idx) {
> > - *idx = index;
> > + *idx = id;
> > }
> > - return &ms->possible_cpus->cpus[index];
> > + return &ms->possible_cpus->cpus[id];
> > }
> >
> > static void spapr_set_vsmt_mode(sPAPRMachineState *spapr, Error **errp)
> > @@ -2514,7 +2512,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> > error_report("This machine version does not support CPU
> > hotplug");
> > exit(1);
> > }
> > - boot_cores_nr = possible_cpus->len;
> > + boot_cores_nr = possible_cpus->len / smp_threads;
> > }
> >
> > if (smc->pre_2_10_has_unused_icps) {
> > @@ -2528,7 +2526,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> > }
> > }
> >
> > - for (i = 0; i < possible_cpus->len; i++) {
> > + for (i = 0; i < possible_cpus->len / smp_threads; i++) {
> > int core_id = i * smp_threads;
> >
> > if (mc->has_hotpluggable_cpus) {
> > @@ -3795,21 +3793,16 @@ spapr_cpu_index_to_props(MachineState *machine,
> > unsigned cpu_index)
> >
> > static int64_t spapr_get_default_cpu_node_id(const MachineState *ms, int
> > idx)
> > {
> > - return idx / smp_cores % nb_numa_nodes;
> > + return idx / (smp_cores * smp_threads) % nb_numa_nodes;
> > }
> >
> > static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState
> > *machine)
> > {
> > int i;
> > const char *core_type;
> > - int spapr_max_cores = max_cpus / smp_threads;
> > - MachineClass *mc = MACHINE_GET_CLASS(machine);
> >
> > - if (!mc->has_hotpluggable_cpus) {
> > - spapr_max_cores = QEMU_ALIGN_UP(smp_cpus, smp_threads) /
> > smp_threads;
> > - }
> > if (machine->possible_cpus) {
> > - assert(machine->possible_cpus->len == spapr_max_cores);
> > + assert(machine->possible_cpus->len == max_cpus);
> > return machine->possible_cpus;
> > }
> >
> > @@ -3820,16 +3813,16 @@ static const CPUArchIdList
> > *spapr_possible_cpu_arch_ids(MachineState *machine)
> > }
> >
> > machine->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
> > - sizeof(CPUArchId) * spapr_max_cores);
> > - machine->possible_cpus->len = spapr_max_cores;
> > + sizeof(CPUArchId) * max_cpus);
> > + machine->possible_cpus->len = max_cpus;
> > for (i = 0; i < machine->possible_cpus->len; i++) {
> > - int core_id = i * smp_threads;
> > -
> > machine->possible_cpus->cpus[i].type = core_type;
> > machine->possible_cpus->cpus[i].vcpus_count = smp_threads;
> > - machine->possible_cpus->cpus[i].arch_id = core_id;
> > + machine->possible_cpus->cpus[i].arch_id = i;
> > machine->possible_cpus->cpus[i].props.has_core_id = true;
> > - machine->possible_cpus->cpus[i].props.core_id = core_id;
> > + machine->possible_cpus->cpus[i].props.core_id = i / smp_threads;
> > + machine->possible_cpus->cpus[i].props.has_thread_id = true;
> > + machine->possible_cpus->cpus[i].props.thread_id = i % smp_threads;
> > }
> > return machine->possible_cpus;
> > }
>
[Qemu-devel] [RFC 2/4] numa: exit on incomplete CPU mapping, Laurent Vivier, 2019/02/12
[Qemu-devel] [RFC 3/4] numa: move cpu_slot_to_string() upper in the function, Laurent Vivier, 2019/02/12
[Qemu-devel] [RFC 4/4] numa: check threads of the same core are on the same node, Laurent Vivier, 2019/02/12