Re: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for phys_to_nid

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2022 09:20, Wei Chen wrote:
> Hi Jan,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年7月11日 14:32
>> To: Wei Chen <Wei.Chen@xxxxxxx>
>> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George
>> Dunlap <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
>> Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Jiamei Xie
>> <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON
>> for phys_to_nid
>>
>> On 08.07.2022 16:54, Wei Chen wrote:
>>> VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This
>>> results in two lines of error-checking code in phys_to_nid
>>> that is not actually working and causing two compilation
>>> errors:
>>> 1. error: "MAX_NUMNODES" undeclared (first use in this function).
>>>    This is because in the common header file, "MAX_NUMNODES" is
>>>    defined after the common header file includes the ARCH header
>>>    file, where phys_to_nid has attempted to use "MAX_NUMNODES".
>>>    This error was resolved after we moved the phys_to_nid from
>>>    x86 ARCH header file to common header file.
>>> 2. error: wrong type argument to unary exclamation mark.
>>>    This is because, the error-checking code contains !node_data[nid].
>>>    But node_data is a data structure variable, it's not a pointer.
>>>
>>> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to
>>> enable the two lines of error-checking code. And fix the left
>>> compilation errors by replacing !node_data[nid] to
>>> !node_data[nid].node_spanned_pages. Although NUMA allows one node
>>> can only have CPUs but without any memory. And node with 0 bytes
>>> of memory might have an entry in memnodemap[] theoretically. But
>>> that doesn't mean phys_to_nid can find any valid address from a
>>> node with 0 bytes memory.
>>>
>>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>>> Tested-by: Jiamei Xie <jiamei.xie@xxxxxxx>
>>> ---
>>> v1 -> v2:
>>> 1. Move from part#1 to part#2. (Comment from NUMA part1 series)
>>> 2. Refine the justification of using
>>>    !node_data[nid].node_spanned_pages. (From NUMA part1 series)
>>> 3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid.
>>> 4. Adjust the conditional express for ASSERT.
>>> 5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86.
>>> 6. Use conditional macro to gate MAX_NUMNODES for other architectures.
>>
>> The change looks okay, but can you please clarify what these last two
>> points describe? They don't seem to match any change ...
>>
> 
> I moved this patch form Part#1 to Part#2, but forgot to remove these
> two change logs. In Part#1, we do those changes, but after we moved
> this patch to Part#2, those changes are unnecessary. I will remove
> these two lines of change logs. Sorry for the confusion!

With this clarified
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan