| Message ID | 1467373826-110305-4-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mst@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@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); } 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. Thanks, drew
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 <mst@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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; > - 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); > - } > + 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 */ > +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; > +}
On Fri, 1 Jul 2016 14:30:12 +0200 Andrew Jones <drjones@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 <mst@redhat.com> > > Signed-off-by: Igor Mammedov <imammedo@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
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 <mst@redhat.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Nice, but please make it return a negative return code on error. > --- > 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; > - 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); > - } > + 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 */ > +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
On Mon, Jul 04, 2016 at 09:06:16AM +0200, Igor Mammedov wrote: > On Fri, 1 Jul 2016 14:30:12 +0200 > Andrew Jones <drjones@redhat.com> wrote: > > On Fri, Jul 01, 2016 at 01:50:24PM +0200, Igor Mammedov wrote: > > > --- 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. I see. So how about creating static inline bool numa_node_is_valid(int node) { return node >= 0 && node < nb_numa_nodes; } and then using that for the condition, instead of the inequality. It's not a huge improvement, so take it or leave it. Thanks, drew
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; - 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); - } + 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 */ +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; +}
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 <mst@redhat.com> Signed-off-by: Igor Mammedov <imammedo@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(-)