On Fri, 1 Jul 2016 14:30:12 +0200 Andrew Jones <drjo...@redhat.com> wrote:
> On Fri, Jul 01, 2016 at 01:50:24PM +0200, Igor Mammedov wrote: > > Replace repeated pattern > > > > for (i = 0; i < nb_numa_nodes; i++) { > > if (test_bit(idx, numa_info[i].node_cpu)) { > > ... > > break; > > > > with a helper function to lookup numa node index for cpu. > > > > Suggested-by: Michael S. Tsirkin <m...@redhat.com> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > --- > > hw/arm/virt-acpi-build.c | 6 ++---- > > hw/arm/virt.c | 7 +++---- > > hw/i386/acpi-build.c | 7 ++----- > > hw/i386/pc.c | 8 +++----- > > hw/ppc/spapr_cpu_core.c | 6 ++---- > > include/sysemu/numa.h | 3 +++ > > numa.c | 12 ++++++++++++ > > 7 files changed, 27 insertions(+), 22 deletions(-) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 28fc59c..5923b3d 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -426,11 +426,9 @@ build_srat(GArray *table_data, BIOSLinker > > *linker, VirtGuestInfo *guest_info) uint32_t *cpu_node = > > g_malloc0(guest_info->smp_cpus * sizeof(uint32_t)); > > for (i = 0; i < guest_info->smp_cpus; i++) { > > - for (j = 0; j < nb_numa_nodes; j++) { > > - if (test_bit(i, numa_info[j].node_cpu)) { > > + j = numa_get_node_for_cpu(i); > > + if (j < nb_numa_nodes) { > > cpu_node[i] = j; > > I think this, and all other occurrences, would read nicer like > > if (numa_enabled()) { > cpu_node[i] = numa_get_node_for_cpu(i); > } it would be nicer but it could be a guest visible change as in case of if cpu is not in numa_info[].node_cpu then old code won't do assignment, if done as suggested it will assign whatever value numa_get_node_for_cpu(i) returns. > > We just need to also add > > bool numa_enabled() > { > return nb_numa_nodes != 0; > } > > > - break; > > - } > > } > > } > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index c5c125e..b066f15 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -411,10 +411,9 @@ static void fdt_add_cpu_nodes(const > > VirtBoardInfo *vbi) armcpu->mp_affinity); > > } > > > > - for (i = 0; i < nb_numa_nodes; i++) { > > - if (test_bit(cpu, numa_info[i].node_cpu)) { > > - qemu_fdt_setprop_cell(vbi->fdt, nodename, > > "numa-node-id", i); > > We were missing the break here, which I had queued to send some day, > now I don't need to :-) > > > - } > > + i = numa_get_node_for_cpu(cpu); > > + if (i < nb_numa_nodes) { > > + qemu_fdt_setprop_cell(vbi->fdt, nodename, > > "numa-node-id", i); } > > > > g_free(nodename); > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 5a594be..60be550 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -2344,18 +2344,15 @@ build_srat(GArray *table_data, BIOSLinker > > *linker, MachineState *machine) srat->reserved1 = cpu_to_le32(1); > > > > for (i = 0; i < apic_ids->len; i++) { > > - int j; > > + int j = numa_get_node_for_cpu(i); > > int apic_id = apic_ids->cpus[i].arch_id; > > > > core = acpi_data_push(table_data, sizeof *core); > > core->type = ACPI_SRAT_PROCESSOR_APIC; > > core->length = sizeof(*core); > > core->local_apic_id = apic_id; > > - for (j = 0; j < nb_numa_nodes; j++) { > > - if (test_bit(i, numa_info[j].node_cpu)) { > > + if (j < nb_numa_nodes) { > > core->proximity_lo = j; > > - break; > > - } > > } > > memset(core->proximity_hi, 0, 3); > > core->local_sapic_eid = 0; > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 44a8f3b..fef34e7 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -780,11 +780,9 @@ static FWCfgState > > *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) for (i = > > 0; i < max_cpus; i++) { unsigned int apic_id = > > x86_cpu_apic_id_from_index(i); assert(apic_id < > > pcms->apic_id_limit); > > - for (j = 0; j < nb_numa_nodes; j++) { > > - if (test_bit(i, numa_info[j].node_cpu)) { > > - numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > > - break; > > - } > > + j = numa_get_node_for_cpu(i); > > + if (j < nb_numa_nodes) { > > + numa_fw_cfg[apic_id + 1] = cpu_to_le64(j); > > } > > } > > for (i = 0; i < nb_numa_nodes; i++) { > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 3a5da09..030016c 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -69,11 +69,9 @@ void spapr_cpu_init(sPAPRMachineState *spapr, > > PowerPCCPU *cpu, Error **errp) } > > > > /* Set NUMA node for the added CPUs */ > > - for (i = 0; i < nb_numa_nodes; i++) { > > - if (test_bit(cs->cpu_index, numa_info[i].node_cpu)) { > > + i = numa_get_node_for_cpu(cs->cpu_index); > > + if (i < nb_numa_nodes) { > > cs->numa_node = i; > > - break; > > - } > > } > > > > xics_cpu_setup(spapr->icp, cpu); > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h > > index bb184c9..4da808a 100644 > > --- a/include/sysemu/numa.h > > +++ b/include/sysemu/numa.h > > @@ -32,4 +32,7 @@ void numa_set_mem_node_id(ram_addr_t addr, > > uint64_t size, uint32_t node); void > > numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t > > node); uint32_t numa_get_node(ram_addr_t addr, Error **errp); > > +/* on success returns node index in numa_info, > > + * on failure returns nb_numa_nodes */ > > Or zero when nb_numa_nodes == 0, which requires another > condition to disambiguate it from node=0. You could > return -1 for both failure to find the node and for > when nb_numa_nodes == 0, but no users are checking > for error right now, only for nb_numa_nodes == 0, which, > as I said above, would look better with numa_enabled() > > > +int numa_get_node_for_cpu(int idx); > > #endif > > diff --git a/numa.c b/numa.c > > index 572712c..5790858 100644 > > --- a/numa.c > > +++ b/numa.c > > @@ -548,3 +548,15 @@ MemdevList *qmp_query_memdev(Error **errp) > > object_child_foreach(obj, query_memdev, &list); > > return list; > > } > > + > > +int numa_get_node_for_cpu(int idx) > > +{ > > + int i; > > + > > + for (i = 0; i < nb_numa_nodes; i++) { > > + if (test_bit(idx, numa_info[i].node_cpu)) { > > + break; > > + } > > + } > > + return i; > > +} > > -- > > 1.8.3.1 > > > > > > numa.c has one of these patterns too in numa_post_machine_init that > could be replaced. I didn't touch it intentionally as numa_post_machine_init is broken code when combined with sparse cpu_index and I plan to remove it in favor of explicit assignment (spapr does it, pc needs to be taught) or may be event redo sole user of CPUState:numa_node (HMP command info numa) and drop CPUState:numa_node field along with numa_post_machine_init() but all that said it's topic for another not related patch. > > Thanks, > drew