Hi Jan,
On 2022/4/26 17:02, Jan Beulich wrote:
On 18.04.2022 11:07, 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 when we moved the definition of
"MAX_NUMNODES" to x86 ARCH header file. And we reserve the
"MAX_NUMNODES" definition in common header file through a
conditional compilation for some architectures that don't
need to define "MAX_NUMNODES" in their ARCH header files.
No, that's setting up a trap for someone else to fall into, especially
with the #ifdef around the original definition. Afaict all you need to
do is to move that #define ahead of the #include in xen/numa.h. Unlike
functions, #define-s can reference not-yet-defined identifiers.
I had tried it before. MAX_NUMNODES depends on NODE_SHIFT. But
NODE_SHIFT depends on the definition status in asm/numa.h. If I move
MAX_NUMNODES to before asm/numa.h, then I have to move NODES_SHIFT as
well. But this will break the original design. NODES_SHIFT in xen/numa.h
will always be defined before asm/numa.h. This will be a duplicated
definition error.
How about I move MAX_NUMNODES to arm and x86 asm/numa.h in this patch
at the same time? Because in one of following patches, MAX_NUMNODES and
phys_to_nid will be moved to xen/numa.h at the same time?
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.
Because when node_spanned_pages is 0, this node has no memory,
numa_scan_node will print warning message for such kind of nodes:
"Firmware Bug or mis-configured hardware?".
This warning is bogus - nodes can have only processors. Therefore I'd
like to ask that you don't use it for justification. And indeed you
Yes, you're right, node can only has CPUs! I will remove it.
don't need to: phys_to_nid() is about translating an address. The
input address can't be valid if it maps to a node with no memory.
Can I understand your comment:
Any input address is invalid, when node_spanned_pages is zero, because
this node has no memory?
Thanks,
Wei Chen
Jan
