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

From: George Dunlap <george.dunlap@cloud.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	 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>,  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 14:15:17 +0000	[thread overview]
Message-ID: <CA+zSX=YHW3kGFroNDzwQg=EhEe3F_fw3gCd_9W+P2UxC7+g+0A@mail.gmail.com> (raw)
In-Reply-To: <4a1f86c7-6643-4fd1-ba1c-a4f86abb63f3@suse.com>

On Thu, Dec 14, 2023 at 8:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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;

In general, it is unreasonable to expect other people to come up with
suggestions to make you happy.  You're ultimately the only person who
knows what would make you satisfied.  You're also more senior and know
the code better, and thus in a better position to be able to come up
with ideas.  "What about X?" "No." "What about Y?" "No." "What about
Z?" "No." Contributors experience it as caustic behavior.

The only time it's acceptable is if you can specify, precisely and
reasonably completely, your criteria for acceptance, such that there's
a clear way forward.  In this case, for instance, it sounds like "Has
a TODO which isn't technically inaccurate" might be the criteria.

In which case, for instance, a solution might be along the lines of:
"TODO: When NUMA is supported on Arm we'll need to do something about
defining first_valid_mfn, which is used by the dummy helpers."  This
punts the actual solution down the road until we need it.

But I do think that it's fair to ask Julien to think about a suitable
wording, since the comment is in a sense to remind him (or other ARM
maintainers) what's needed, and since the eventual solution will be
something to do with the ARM code and architecture anyway.

 -George

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