Re: [PATCH v5] numa: make node_to_cpumask_map() NUMA_NO_NODE aware

On 2019/9/17 18:08, Michal Hocko wrote:

On Tue 17-09-19 17:53:57, Yunsheng Lin wrote:

quoted

On 2019/9/17 17:36, Michal Hocko wrote:

quoted

On Tue 17-09-19 14:20:11, Yunsheng Lin wrote:

quoted

On 2019/9/17 13:28, Michael Ellerman wrote:

quoted

Yunsheng Lin [off-list ref] writes:

[...]

quoted

quoted

quoted

But we cannot really copy the page allocator logic. Simply because the
page allocator doesn't enforce the near node affinity. It just picks it
up as a preferred node but then it is free to fallback to any other numa
node. This is not the case here and node_to_cpumask_map will only restrict
to the particular node's cpus which would have really non deterministic
behavior depending on where the code is executed. So in fact we really
want to return cpu_online_mask for NUMA_NO_NODE.

Some arches were already NUMA_NO_NODE aware, but they return cpu_all_mask,
which should be identical with cpu_online_mask when those arches do not
support cpu hotplug, this patch also changes them to return cpu_online_mask
in order to be consistent and use NUMA_NO_NODE instead of "-1".

Except some of those arches *do* support CPU hotplug, powerpc and sparc
at least. So switching from cpu_all_mask to cpu_online_mask is a
meaningful change.

Yes, thanks for pointing out.

quoted

That doesn't mean it's wrong, but you need to explain why it's the right
change.

How about adding the below to the commit log:
Even if some of the arches do support CPU hotplug, it does not make sense
to return the cpu that has been hotplugged.

Any suggestion?

Again, for the third time, I believe. Make it a separate patch please.
There is absolutely no reason to conflate those two things.

Ok, thanks.
Will make the cpu_all_mask -> cpu_online_mask change a separate patch.

Thanks. This really needs per arch maintainer to check closely.

quoted

Also, do you think it is better to resend this as individual patches for each arch
or have all these changes in a single patch? I am not sure which is the common
practice for a multi-arches changes like this.

It really depends on arch maintainers. Both approaches have some pros
and cons. A single patch is more compact and and parts are not going to
get lost in noise. They might generate some conflicts with parallel
changes. I suspect a conflict risk is quite low in this code considering
from a recent activity. A follow up arch specific patch would have to be
routed via Andrew as well.

If Andrew is ok routing it through his tree and none of the arch
maintainers is opposed then I would go with a single patch.

Ok, I will try a single patch for NUMA_NO_NODE aware change first.
"cpu_all_mask -> cpu_online_mask" change seems a little controversial,
and may need deeper investigation.