Re: [PATCH v4 5/6] xen/x86: move NUMA scan nodes codes from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2022 17:26, Wei Chen wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年9月8日 21:03
>>
>> On 02.09.2022 05:31, Wei Chen wrote:
>>> --- a/xen/arch/x86/numa.c
>>> +++ b/xen/arch/x86/numa.c
>>> @@ -41,9 +41,12 @@ int __init arch_numa_setup(const char *opt)
>>>      return -EINVAL;
>>>  }
>>>
>>> -bool arch_numa_disabled(void)
>>> +bool arch_numa_disabled(bool init_as_disable)
>>
>> I'm afraid my question as to the meaning of the name of the parameter has
>> remained unanswered.
>>
> 
> Sorry, I might missed some contents of your reply in v3. The name of this
> parameter has been bothering me for a long time, and now this is actually
> quite awkward. The origin of this parameter is because the current NUMA
> implementation will make different judgments under different usage
> conditions when using acpi_numa. In acpi_scan_nodes, it uses acpi_numa <= 0
> as the condition for judging that ACPI NUMA is turned off. But only use
> acpi_numa < 0 as condition in srat_disabled and elsewhere. I use this
> parameter in the hope that we can keep the same semantics as the original
> code without changing the code of the caller.

The difference is "bad only" vs "bad or no data". Maybe that's easier
to express via two functions - arch_numa_disabled() (checking <= 0)
and arch_numa_broken() (checking < 0)? With a single function I guess
the name of the parameter would always be clumsy at best. Unless
someone has a good idea for a suitable name ...

>>> --- a/xen/drivers/acpi/Kconfig
>>> +++ b/xen/drivers/acpi/Kconfig
>>> @@ -7,4 +7,5 @@ config ACPI_LEGACY_TABLES_LOOKUP
>>>
>>>  config ACPI_NUMA
>>>     bool
>>> +   select HAS_NUMA_NODE_FWID
>>
>> Are you selecting an option here which doesn't exist anywhere? Or
>> am I overlooking where this new option is being added?
>>
> 
> Yes, this is a new Kconfig option. Should I need to introduce in a
> separate patch?

I don't think that'll need to be in a separate patch; it can simply
be another hunk in the one here, adding the needed 2 lines (plus a
blank one) to, presumably, common/Kconfig.

Jan