Re: [PATCH v2 2/9] xen/x86: Use enumerations to indicate NUMA status

[Jan Beulich <jbeulich@xxxxxxxx>]

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