On 12.07.2022 09:20, Wei Chen wrote: > Hi Jan, > >> -----Original Message----- >> From: Jan Beulich <[email protected]> >> Sent: 2022年7月11日 14:32 >> To: Wei Chen <[email protected]> >> Cc: nd <[email protected]>; Andrew Cooper <[email protected]>; George >> Dunlap <[email protected]>; Julien Grall <[email protected]>; Stefano >> Stabellini <[email protected]>; Wei Liu <[email protected]>; Jiamei Xie >> <[email protected]>; [email protected] >> 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 <[email protected]> >>> Tested-by: Jiamei Xie <[email protected]> >>> --- >>> 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 <[email protected]> Jan
