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

[Julien Grall <julien.grall@arm.com>]

On 25/07/17 07:47, Vijay Kilari wrote:
>>>  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.

I don't really want to see error code and error handling everywhere in 
the initialization code. I would assume that if the NUMA bindings are 
wrong we should just crash Xen rather continuing with NUMA disabled.

Stefano do you have any opinion here?

[...]

>>>  #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().

This does not answer question... Please read it again and explain why 
you can't do:

nodeid_t cpu_to_nodemap[NR_CPUS];

>>> 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.

You should make common only functions that will be called from code 
common or are part of common code.

In this particular case, the name might be the same but the behavior is 
completely different and the way to use it too...

Cheers,

-- 
Julien Grall

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