Re: [PATCH v3 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2022 11:49, Wei Chen wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年8月25日 18:58
>>
>> On 22.08.2022 04:58, Wei Chen wrote:
>>> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>>> +
>>> +bool __read_mostly numa_off;
>>
>> This, otoh, can be, or have I missed a place where it's written by a
>> non-__init function?
>>
> 
> I think yes, it will be used in numa_disabled and numa_disabled will
> be called in cpu_add.

In the original code I cannot spot such a path - can you please point
out how exactly you see numa_disabled() reachable from cpu_add()? I'm
clearly overlooking something ...

>>> +bool numa_disabled(void)
>>> +{
>>> +    return numa_off || arch_numa_disabled(false);
>>> +}
>>> +
>>> +/*
>>> + * Given a shift value, try to populate memnodemap[]
>>> + * Returns :
>>> + * 1 if OK
>>> + * 0 if memnodmap[] too small (of shift too small)
>>> + * -1 if node overlap or lost ram (shift too big)
>>> + */
>>> +static int __init populate_memnodemap(const struct node *nodes,
>>> +                                      nodeid_t numnodes, unsigned int
>> shift,
>>
>> I don't think you can use nodeid_t for a variable holding a node count.
>> Think of what would happen if there were 256 nodes, the IDs of which
>> all fit in nodeid_t. (Same again further down.)
>>
> 
> If we use u8 as nodeid_t, why there will be 256 nodes to here?
> And the MAX_NUMNODES has been limited to 64 (using NODES_SHIFT or
> CONFIG_NR_NUMA_NODES). If we allow 256 nodes, we have to update MAX_NUMNODES
> and nodeid_t first I think?

Well, when writing the reply I did forget about MAX_NUMNODES, so yes,
with that the value can't be larger than 255. Nevertheless I don't
think a count-of-nodes value should be expressed with nodeid_t. It
should be simply "unsigned int".

>>> +                                      nodeid_t *nodeids)
>>> +{
>>> +    unsigned long spdx, epdx;
>>> +    nodeid_t i;
>>
>> This is likely inefficient for a loop counter variable. Note how you
>> use "unsigned int" in e.g. extract_lsb_from_nodes().
>>
> 
> Did you mean u8 for "i" will cause something like unalignment, and will
> cause loop inefficient. If yes, I will use unsigned int for "i" in next
> version.

There's no issue with mis-alignment afaics, but there still is the
inefficiency issue requiring the loop variable to be zero-extended
before being usable as an array. Both x86-64 and aarch64 have the
zero-extension as a side effect when moving 32-bit quantities (from
memory or between registers), and arm wouldn't require any zero-
extension at all then.

Jan