On 08.07.2022 16:54, Wei Chen wrote:
> In current code, x86 is using two variables, numa_off and acpi_numa,
> to indicate the NUMA status. This is because NUMA is not coupled with
> ACPI, and ACPI still can work without NUMA on x86. With these two
> variables' combinations, x86 can have several NUMA status:
> NUMA swith on,
> NUMA swith off,
> NUMA swith on with NUMA emulation,
> NUMA swith on with no-ACPI,
> NUMA swith on with ACPI.
Hmm, with both this and the actual change I'm not able to convince
myself that you've expressed the prior combinations correctly. May
I suggest that you make table representing the 6 (I think)
combinations of original states with their mapping to the new
enumerators? (It doesn't need to be 6 different enumerators, but
all 6 existing states need a [proper] representation in the new
model.)
As an aside - I think you mean "switched" in all five of these
lines.
> --- a/xen/arch/x86/include/asm/numa.h
> +++ b/xen/arch/x86/include/asm/numa.h
> @@ -28,12 +28,22 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
> #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
> #define VIRTUAL_BUG_ON(x)
>
> +/* Enumerations for NUMA status. */
> +enum numa_mode {
> + numa_on = 0,
> + numa_off,
May I suggest to switch these two around, such that "off" becomes
the meaning of 0, potentially allowing ! to be used in a boolean-
like fashion here or there? And please omit the "= 0" part - it's
only non-zero first values which actually need spelling out.
> + /* NUMA turns on, but ACPI table is bad or disabled. */
> + numa_no_acpi,
> + /* NUMA turns on, and ACPI table works well. */
> + numa_acpi,
As to the names of these: In the description you already say that
you want to re-use the code for non-ACPI cases. Wouldn't you better
avoid "acpi" in the names then (rather than perhaps renaming these
another time later on)?
I'd also like to understand what useful state "numa_no_acpi" is.
I realize this was a state expressable by the two original
variables, but does it make sense?
> @@ -528,7 +528,8 @@ int __init acpi_scan_nodes(paddr_t start, paddr_t end)
> for (i = 0; i < MAX_NUMNODES; i++)
> cutoff_node(i, start, end);
>
> - if (acpi_numa <= 0)
> + /* Only when numa_on with good firmware, we can do numa scan nodes. */
> + if (!numa_enabled_with_firmware())
> return -1;
Nit: Perhaps drop "do numa" from the comment?
Jan
