Re: [PATCH 1/2] include/linux/nodemask.h: create node_available() helper

["liupeng (DM)" <liupeng256@huawei.com>]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

On 2022/5/7 1:23, Bjorn Helgaas wrote:
> Subject line convention looks like "numa: ..."
>
> On Fri, May 06, 2022 at 01:58:00AM +0000, Peng Liu wrote:
>> Lots of code dose
>                 does
>
>> 	node != NUMA_NO_NODE && !node_online(node)
>> or
>> 	node == NUMA_NO_NODE || node_online(node)
>> so create node_available to do this to simplify code.
>              node_available()

Thanks.

> I'm not really sure what meaning "node_available" conveys, though.
> Probably just because I don't understand NUMA.
>
> Should the test for NUMA_NO_NODE be folded into node_state() or
> node_online() directly instead of adding a new node_available()
> interface?
>
> NUMA_NO_NODE is -1.  It's not clear to me that node_state()/
> node_isset()/test_bit() would do the right thing given -1.  I doubt
> all node_online() callers ensure they don't pass NUMA_NO_NODE.

I have tested node_online(NUMA_NO_NODE) on x86_64, arm64, arm32, the results are:

arch		node_online(NUMA_NO_NODE)
------------------------------------------
x86_64		0
arm64		1
arm32		0
.

Hence, I think the behavior of node_state(NUMA_NO_NODE) is undefined which is due to
test_bit(NUMA_NO_NODE) is different between different arches.

For many times, callers could ensure they don't pass NUMA_NO_NODE, for example "nid < 0" is
checked before node_online() or some code only used in NUMA has node. So, it is not
suitable to test NUMA_NO_NODE in node_state() or node_online().

Unfortunately, there are actually some cases that test for NUMA_NO_NODE is needed but ignored.
Related issue: ab31c7fd2d37 ("sched/numa: Fix boot crash on arm64 systems")

>> --- a/include/linux/nodemask.h
>> +++ b/include/linux/nodemask.h
>> @@ -70,6 +70,7 @@
>>    *
>>    * int node_online(node)		Is some node online?
>>    * int node_possible(node)		Is some node possible?
>> + * int node_available(node)		Is some node available(online or NUMA_NO_NODE)?
> Existing file generally fits in 80 columns; follow that lead unless
> you have a really good reason.  E.g., maybe this?
>
>    + * int node_available(node)		Node online or NUMA_NO_NODE
> .

Thank you for your suggestion, I will send a revised patch with modified comments later.

[-- Attachment #2: Type: text/html, Size: 3230 bytes --]