Re: [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support

From: Vijay Kilari <vijay.kilari@gmail.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Tim Deegan <tim@xen.org>,
	kevin.tian@intel.com, Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dario Faggioli <dario.faggioli@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Subject: Re: [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support
Date: Tue, 25 Jul 2017 12:17:08 +0530	[thread overview]
Message-ID: <CALicx6tp7zU2NSd3USgcxixBv3HYO8QqGVT+=xQhkq66AVyx7Q@mail.gmail.com> (raw)
In-Reply-To: <1b4adfe8-c66d-b701-a6ee-815fb7b38734@arm.com>

Hi Julien,

On Mon, Jul 24, 2017 at 4:54 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Vijay,
>
>
> On 18/07/17 12:41, vijay.kilari@gmail.com wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> For each cpu, update cpu_to_node[] with node id from
>> the numa-node-id DT property. Also, initialize cpu_to_node[]
>> with node 0.
>>
>> Add macros to access cpu_to_node[] information.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> ---
>> v3: - Dropped numa_add_cpu declaration from asm-arm/numa.h
>>     - Dropped stale declarations
>>     - Call numa_add_cpu for cpu0
>> ---
>>  xen/arch/arm/numa/numa.c   | 21 +++++++++++++++++++++
>>  xen/arch/arm/setup.c       |  2 ++
>>  xen/arch/arm/smpboot.c     | 25 ++++++++++++++++++++++++-
>>  xen/include/asm-arm/numa.h |  7 +++++++
>>  xen/include/asm-x86/numa.h |  1 -
>>  xen/include/xen/numa.h     |  1 +
>>  6 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/numa/numa.c b/xen/arch/arm/numa/numa.c
>> index c00b92c..dc80aa5 100644
>> --- a/xen/arch/arm/numa/numa.c
>> +++ b/xen/arch/arm/numa/numa.c
>> @@ -22,11 +22,31 @@
>>
>>  static uint8_t (*node_distance_fn)(nodeid_t a, nodeid_t b);
>>
>> +/*
>> + * Setup early cpu_to_node.
>> + */
>> +void __init init_cpu_to_node(void)
>> +{
>> +    int i;
>> +
>> +    for ( i = 0; i < NR_CPUS; i++ )
>> +        numa_set_node(i, 0);
>> +}
>
>
> From the comment: "Setup early cpu_to_node". However this is not how you are
> using it.

Ok. I will update the comment.

>
> But I am not sure why it is even here...
>
>> +
>>  void numa_failed(void)
>>  {
>>      numa_off = true;
>>      init_dt_numa_distance();
>>      node_distance_fn = NULL;
>> +    init_cpu_to_node();
>> +}
>> +
>> +void __init numa_set_cpu_node(int cpu, unsigned int nid)
>> +{
>> +    if ( !node_isset(nid, processor_nodes_parsed) || nid >= MAX_NUMNODES
>> )
>> +        nid = 0;
>
>
> This looks wrong to me. If the node-id is invalid, why would you blindly set
> to 0?

Generally this check will not pass. I will make this function return
error code in case
of wrong nid.

>
>
>> +
>> +    numa_set_node(cpu, nid);
>>  }
>>
>>  uint8_t __node_distance(nodeid_t a, nodeid_t b)
>> @@ -49,6 +69,7 @@ void __init numa_init(void)
>>      int ret = 0;
>>
>>      nodes_clear(processor_nodes_parsed);
>> +    init_cpu_to_node();
>>      init_dt_numa_distance();
>>
>>      if ( numa_off )
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index a6d1499..b9c8b0d 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -787,6 +787,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>      processor_id();
>>
>> +    numa_add_cpu(0);
>> +
>>      smp_init_cpus();
>>      cpus = smp_get_max_cpus();
>>      printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", cpus);
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 32e8722..fcf9afc 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -29,6 +29,7 @@
>>  #include <xen/timer.h>
>>  #include <xen/irq.h>
>>  #include <xen/console.h>
>> +#include <xen/numa.h>
>
>
> Please use the alphabetical order.
>
>>  #include <asm/cpuerrata.h>
>>  #include <asm/gic.h>
>>  #include <asm/psci.h>
>> @@ -106,6 +107,7 @@ static void __init dt_smp_init_cpus(void)
>>          [0 ... NR_CPUS - 1] = MPIDR_INVALID
>>      };
>>      bool_t bootcpu_valid = 0;
>> +    nodeid_t *cpu_to_nodemap;
>>      int rc;
>>
>>      mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
>> @@ -117,11 +119,18 @@ static void __init dt_smp_init_cpus(void)
>>          return;
>>      }
>>
>> +    cpu_to_nodemap = xzalloc_array(nodeid_t, NR_CPUS);
>
>
> Why do you need to allocate cpu_to_nodemap? Would not it be easier to put it
> on the stack as we do for other variable?

This array holds nodemap indexed by cpuid once for all the cpus.
Later while setting the logical cpu id mapping, the node mapping is set
by calling numa_set_cpu_node().

>
>> +    if ( !cpu_to_nodemap )
>> +    {
>> +        printk(XENLOG_WARNING "Failed to allocate memory for
>> cpu_to_nodemap\n");
>> +        return;
>> +    }
>> +
>>      dt_for_each_child_node( cpus, cpu )
>>      {
>>          const __be32 *prop;
>>          u64 addr;
>> -        u32 reg_len;
>> +        uint32_t reg_len, nid;
>>          register_t hwid;
>>
>>          if ( !dt_device_type_is_equal(cpu, "cpu") )
>> @@ -146,6 +155,15 @@ static void __init dt_smp_init_cpus(void)
>>              continue;
>>          }
>>
>> +        if ( !dt_property_read_u32(cpu, "numa-node-id", &nid) )
>> +        {
>> +            printk(XENLOG_WARNING "cpu node `%s`: numa-node-id not
>> found\n",
>> +                   dt_node_full_name(cpu));
>
>
> numa-node-id is not mandatory. So you would print a warning on all non-NUMA
> platform. This not what we want.

ok. I will drop this warning.
>
>> +            nid = 0;
>> +        }
>> +
>> +        cpu_to_nodemap[cpuidx] = nid;
>> +
>>          addr = dt_read_number(prop, dt_n_addr_cells(cpu));
>>
>>          hwid = addr;
>> @@ -224,6 +242,7 @@ static void __init dt_smp_init_cpus(void)
>>      {
>>          printk(XENLOG_WARNING "DT missing boot CPU MPIDR[23:0]\n"
>>                 "Using only 1 CPU\n");
>> +        xfree(cpu_to_nodemap);
>>          return;
>>      }
>>
>> @@ -233,7 +252,10 @@ static void __init dt_smp_init_cpus(void)
>>              continue;
>>          cpumask_set_cpu(i, &cpu_possible_map);
>>          cpu_logical_map(i) = tmp_map[i];
>> +        numa_set_cpu_node(i, cpu_to_nodemap[i]);
>>      }
>> +
>> +    xfree(cpu_to_nodemap);
>>  }
>>
>>  void __init smp_init_cpus(void)
>> @@ -313,6 +335,7 @@ void start_secondary(unsigned long boot_phys_offset,
>>       */
>>      smp_wmb();
>>
>> +    numa_add_cpu(cpuid);
>
>
> Newline here please.
>
>
>>      /* Now report this CPU is up */
>>      cpumask_set_cpu(cpuid, &cpu_online_map);
>>
>> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
>> index d1dc83a..0d3146c 100644
>> --- a/xen/include/asm-arm/numa.h
>> +++ b/xen/include/asm-arm/numa.h
>> @@ -10,12 +10,19 @@ void init_dt_numa_distance(void);
>>  #ifdef CONFIG_NUMA
>>  void numa_init(void);
>>  int dt_numa_init(void);
>> +void numa_set_cpu_node(int cpu, unsigned int nid);
>> +
>>  #else
>>  static inline void numa_init(void)
>>  {
>>      return;
>>  }
>>
>> +static inline void numa_set_cpu_node(int cpu, unsigned int nid)
>> +{
>> +    return;
>> +}
>> +
>>  /* Fake one node for now. See also node_online_map. */
>>  #define cpu_to_node(cpu) 0
>>  #define node_to_cpumask(node)   (cpu_online_map)
>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>> index ca0a2a6..fc4747f 100644
>> --- a/xen/include/asm-x86/numa.h
>> +++ b/xen/include/asm-x86/numa.h
>> @@ -15,7 +15,6 @@ extern nodeid_t acpi_setup_node(unsigned int pxm);
>>  extern void srat_detect_node(int cpu);
>>
>>  extern nodeid_t apicid_to_node[];
>> -extern void init_cpu_to_node(void);
>>
>>  void srat_parse_regions(paddr_t addr);
>>  unsigned int arch_get_dma_bitsize(void);
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 10ef4c4..8a306e7 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -30,6 +30,7 @@ extern s8 acpi_numa;
>>  void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn);
>>  int srat_disabled(void);
>>  int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
>> +void init_cpu_to_node(void);
>
>
> You never used this function in common code. So why did you move it in the
> common headers?

Same was defined for x86 as well. So I have moved to common header file.

>
>>
>>  #ifdef CONFIG_NUMA
>>  #define cpu_to_node(cpu)         (cpu_to_node[cpu])
>>
>
> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel