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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年9月9日 0:02
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 5/6] xen/x86: move NUMA scan nodes codes from x86
> to common
> 
> 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 ...
> 

Yes, I can't find a good name for the parameter, so break into two functions
would be better, I will do it in next version.

> >>> --- 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.

Ok.

Thanks,
Wei Chen

> 
> Jan