[PATCH] arm64/mm: return cpu_all_mask when node is NUMA_NO_NODE
gshan at redhat.com
Sat Sep 19 20:05:37 EDT 2020
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
> 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];
Yes, the check is needed by the inline function either.
More information about the linux-arm-kernel