[PATCH] arm64/mm: return cpu_all_mask when node is NUMA_NO_NODE

[Zhengyuan Liu]

On 2020/9/18 下午1:40, Gavin Shan wrote:
> Hi Zhengyuan,
> 
> On 9/18/20 11:15 AM, Zhengyuan Liu wrote:
>> Filter out NUMA_NO_NODE before returning cpumask of a node, otherwise
>> it will triger the following WARN_ON(node >= nr_node_ids).
>>
>> Signed-off-by: Zhengyuan Liu <liuzhengyuan at tj.kylinos.cn>
>> ---
>>   arch/arm64/mm/numa.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index 73f8b49d485c..78f9b7dab656 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -46,6 +46,9 @@ EXPORT_SYMBOL(node_to_cpumask_map);
>>    */
>>   const struct cpumask *cpumask_of_node(int node)
>>   {
>> +    if (node == NUMA_NO_NODE)
>> +        return cpu_all_mask;
>> +
>>       if (WARN_ON(node >= nr_node_ids))
>>           return cpu_none_mask;
>>
> 
> I don't understand how NUMA_NO_NODE triggers the warning, as mentioned
> in the change log. NUMA_NO_NODE is defined as (-1), but @nr_node_ids
> is always larger than 0. @cpu_all_mask returned in this case would be
> meaningless. It sounds not reasonable to pass invalid @node to this
> function.

node is defined as int while nr_node_ids is defined as unsigned int, 
when both compare in WARN_ON() node will automatically 
NUMA_NO_NODEconvert to unsigned int and that's why warning is triggered 
here.

> 
> I guess the best way is to fix the issue from the caller. In the mean 
> while,
> to report the invalid NUMA node would be helpful. So I think you might 
> be need.
> Note the change log needs improvement either :)
> 
>      if (WARN_ON(node == NUMA_NO_NODE))
>         return cpu_all_mask;


when run the kernel on UMA machine with CONFIG_NUMA enabled, the node 
property of devices (dev->numa_node) is mainly initialized to 
NUMA_NO_NODE. From this point, NUMA_NO_NODE seems not like a illegal 
value and return cpu_all_mask seems reasonable. Actually, other arch 
like sparc, alpha and powerpc also return cpu_all_mask in 
cpumask_of_node() when passed NUMA_NO_NODE. x86 may need to fix this too.

BTW, I think we should add a check in numa.h too.

--- a/arch/arm64/include/asm/numa.h
+++ b/arch/arm64/include/asm/numa.h
@@ -25,6 +25,9 @@ const struct cpumask *cpumask_of_node(int node);
  /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
  static inline const struct cpumask *cpumask_of_node(int node)
  {
+       if (node == NUMA_NO_NODE)
+               return cpu_all_mask;
+
         return node_to_cpumask_map[node];
  }
  #endif

> 
>      if (WARN_ON(node < 0 || node >= nr_node_ids)
>         return cpu_none_mask;
> 
> More information about @nr_node_ids: it has fixed value (1U) when
> MAX_NUMNODES is smaller than 2. Otherwise, it's initialized to number
> of NUMA nodes, probed from ACPI (SRAT) table, FDT, or dummy initialization.
> For the later case, @nr_node_ids is always larger than 0.

Thanks for explanation, nice for you.

> 
> Cheers,
> Gavin
> 
> 
>