Re: [PATCH v9 3/6] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for phys_to_nid

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2022 11:40, Wei Chen wrote:
> Hi Jan,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年12月13日 17:47
>> 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; Roger Pau Monné
>> <roger.pau@xxxxxxxxxx>
>> Subject: Re: [PATCH v9 3/6] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON
>> for phys_to_nid
>>
>> On 18.11.2022 11:45, 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>
>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> This patch is what is causing the regression found by osstest. The
>> previously silent (dead) checks no trigger when paging_init()
>> encounters a hole in SRAT-described space, as is the case e.g. on
>> the himrods:
>>
>> (XEN) NUMA: Node 0 PXM 0 [0000000000000000, 000000007fffffff]
>> (XEN) NUMA: Node 0 PXM 0 [0000000100000000, 000000087fffffff]
>> (XEN) NUMA: Node 1 PXM 1 [0000000880000000, 000000107fffffff]
>> (XEN) NUMA: Using 19 for the hash shift
>>
>> The node ID for 0x80000000 (slot 1 of memnodemap[]) is NUMA_NO_NODE,
>> which of course fails the left side of ...
>>
> 
> I am sorry, I had not triggered this in my testing machine. I'm a bit
> curious, on NUMA platforms will there be memory that doesn't belong to
> any node? If not, why the space of 0x80000000 will be used in paging_init?

The space isn't used (see the full memory map in the log collected by
osstest), but the function is called early to calculate memflags (which
then isn't used in that particular loop iteration).

Jan