Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: Nicola Vetrini <nicola.vetrini@bugseng.com>,
	consulting@bugseng.com,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	Shawn Anastasio <sanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn
Date: Thu, 14 Dec 2023 09:51:16 +0100	[thread overview]
Message-ID: <4a1f86c7-6643-4fd1-ba1c-a4f86abb63f3@suse.com> (raw)
In-Reply-To: <9fcc73f0-fc9c-4f4f-a431-f1f3b0df1b6a@xen.org>

On 14.12.2023 09:32, Julien Grall wrote:
> Hi Jan,
> 
> On 14/12/2023 07:53, Jan Beulich wrote:
>> On 14.12.2023 03:05, Stefano Stabellini wrote:
>>> On Wed, 13 Dec 2023, Jan Beulich wrote:
>>>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>>>> --- a/xen/arch/arm/include/asm/numa.h
>>>>> +++ b/xen/arch/arm/include/asm/numa.h
>>>>> @@ -2,8 +2,9 @@
>>>>>   #define __ARCH_ARM_NUMA_H
>>>>>   
>>>>>   #include <xen/mm.h>
>>>>
>>>> With this, ...
>>>>
>>>>> +#include <xen/types.h>
>>>>>   
>>>>> -typedef u8 nodeid_t;
>>>>> +typedef uint8_t nodeid_t;
>>>>>   
>>>>>   #ifndef CONFIG_NUMA
>>>>>   
>>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>>>   #define node_to_cpumask(node)   (cpu_online_map)
>>>>>   
>>>>>   /*
>>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>>>>> - * is required because the dummy helpers are using it.
>>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm,
>>>>> + * this is required because the dummy helpers are using it.
>>>>>    */
>>>>> -extern mfn_t first_valid_mfn;
>>>>
>>>> ... and this declaration moved to xen/mm.h, how is it going to be possible
>>>> to do as the adjusted comment says? The compiler will choke on the static
>>>> after having seen the extern.
>>>
>>> Nicola was just following a review comment by Julien. NUMA has been
>>> pending for a while and I wouldn't hold this patch back because of it.
>>> I suggest we go with Julien's request (this version of the patch).
>>>
>>> If you feel strongly about it, please suggest an alternative.
>>
>> Leaving a TODO comment which cannot actually be carried out is just wrong
>> imo. And I consider in unfair to ask me to suggest an alternative. The
>> (imo obvious) alternative is to drop this patch, if no proper change can
>> be proposed. There's nothing wrong with leaving a violation in place,
>> when that violation is far from causing any kind of harm. The more that
>> the place is already accompanied by a (suitable afaict) comment.
> 
> Just to clarify, are you saying you would be happy if the proposal if 
> the TODO is removed?

Removing the bad (new) TODO here is an option. But the previous TODO shouldn't
go away. However, you asking now required me to actually look into Stefano's
request to make an alternative proposal (I still don't see though why it
needed to be me to think about an appropriate solution; probably in the time
I've spent writing replies on this thread, I could have made the change myself).

First, Arm's and PPC's header have this in their !NUMA sections. Much like
Oleksii's putting in quite a bit of effort to reduce redundancy in order to
not further grow it with RISC-V, what's wrong with sorting the !NUMA case
properly as a first step? Move the entire !NUMA section either into xen/numa.h
(eliminating the need for arch-es not supporting NUMA to even have such a
header), or (if need be) to asm-generic/. Then, as a 2nd step (albeit that
could also be done on its own, just the result would be less neat imo), make
the variable in question here extern _only_ when !NUMA. This would then
address the original TODO, so that could then legitimately be dropped. This
would further be a good opportunity to adjust the already stale comment in
page_alloc.c (it's no longer applicable to Arm only).

Jan