Re: [Qemu-devel] [PATCH v3 3/5] numa: reduce code duplication by adding helper numa_get_node_for_cpu()

[Andrew Jones]

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 <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  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