Re: [PATCH v5 1/6] xen/x86: Provide helpers for common code to access acpi_numa

From: Wei Chen <Wei.Chen@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: nd@arm.com, "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v5 1/6] xen/x86: Provide helpers for common code to access acpi_numa
Date: Thu, 29 Sep 2022 14:29:30 +0800	[thread overview]
Message-ID: <1b2c0122-19d0-1478-0b51-6cb685a5273b@arm.com> (raw)
In-Reply-To: <5547335f-9e01-d461-f866-c0f823aa2814@suse.com>

Hi Jan,

On 2022/9/27 15:37, Jan Beulich wrote:
> On 20.09.2022 11:12, Wei Chen wrote:
>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -50,9 +50,28 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>>   bool numa_off;
>>   s8 acpi_numa = 0;
>>   
>> -int srat_disabled(void)
>> +int __init arch_numa_setup(const char *opt)
>>   {
>> -    return numa_off || acpi_numa < 0;
>> +#ifdef CONFIG_ACPI_NUMA
>> +    if ( !strncmp(opt, "noacpi", 6) )
>> +    {
>> +        numa_off = false;
>> +        acpi_numa = -1;
> 
> When making the v5 changes, did you go over the results to check they are
> actually consistent? I'm afraid they still aren't, because of the line
> above: Here we disable NUMA, but that doesn't mean there's broken firmware.

Yes, you're right. I had not realized it while I was modifying this patch.

> Therefore I guess I need to ask for another round of renaming of the two
> helper functions; I'm sorry for that. What you introduce ...
> 
>> +        return 0;
>> +    }
>> +#endif
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +bool arch_numa_broken(void)
>> +{
>> +    return acpi_numa < 0;
>> +}
> 
> ... here wants to be arch_numa_disabled(), whereas the function currently
> named this way (in patch 5) wants to be e.g. arch_numa_unavailable() (or,
> using inverted sense, arch_numa_available()). Of course I'll be happy to
> see other naming suggestions for both functions, as long as they reflect
> the respective purposes.
> 
> Alternatively, to retain the current naming, the assignments to acpi_numa
> would need revising. But I think that would be the more fragile approach.
> 

Yes, I agree with you, I will rename these two functions. Your suggested 
names are reasonable, I will use them in next version.

Cheers,
Wei Chen

> Jan