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

[Gavin Shan]

Hi Zhengyuan,

On 9/18/20 5:03 PM, Zhengyuan Liu wrote:
> On 2020/9/18 下午1:40, Gavin Shan wrote:
>> 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.
> 

Thanks for the explanation, which makes sense to me. Could you
please fold it to the change log?

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

Yes, I don't think WARN_ON is needed for this case where @node is
NUMA_NO_NODE. So the changes look good to me.

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

Yes, the check is needed by the inline function either.

[...]

Thanks,
Gavin