Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/12/2023 13:20, Jan Beulich wrote:
> On 22.12.2023 14:07, Oleksii wrote:
> > On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
> > > On 21.12.2023 20:09, Julien Grall wrote:
> > > > On 20/12/2023 14:08, Oleksii Kurochko wrote:
> > > > > <asm/numa.h> is common through some archs so it is moved
> > > > > to asm-generic.
> > > > >
> > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > > > Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> > > > > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > Acked-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > > > I think this patch will need to be rebased on top of the lastest
> > > > staging
> > > > as this should clash with 51ffb3311895.
> > > No, and I'd like to withdraw my ack here. In this case a stub header
> > > isn't
> > > the right choice imo - the !NUMA case should be handled in common
> > > code. I
> > > would have submitted the patch I have, if only the first_valid_mfn
> > > patch
> > > hadn't been committed already (which I now need to re-base over).
> > I haven't seen your patch yet (waiting for it), but I assume that
> > <asm/numa.h> will still be necessary and remain the same across Arm,
> > PPC, and RISC-V.
> >
> > What am I missing?
> asm/numa.h will be necessary for an arch only if it actually has a way
> to have CONFIG_NUMA enabled. Below, for your reference, the patch in
> the not-yet-rebased form. As you can see, Arm's (and PPC's) header goes
> away. If and when they choose to support NUMA, they may need to regain
> one; whether at that point we can sort how an asm-generic/ form of the
> header might sensibly look like remains to be seen.
>
> Jan
>
> NUMA: no need for asm/numa.h when !NUMA
>
> There's no point in every architecture carrying the same stubs for the
> case when NUMA isn't enabled (or even supported). Move all of that to
> xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
> inclusion of asm/numa.h dependent upon NUMA=y.
>
> Address the TODO regarding first_valid_mfn by making the variable static
> when NUMA=y, thus also addressing a Misrs C:2012 rule 8.4 concern. Drop
> the not really applicable "implement NUMA support" comment - in a !NUMA
> section this simply makes no sense.
I have to admit, I am not really thrill with the approach you have 
taken. As I said before, I don't quite understand the problem of having 
first_valid_mfn exposed in all the circumstances.
This is better than trying to do...

> --- unstable.orig/xen/common/page_alloc.c
> +++ unstable/xen/common/page_alloc.c
> @@ -140,7 +140,6 @@
>   #include <xen/vm_event.h>
>   
>   #include <asm/flushtlb.h>
> -#include <asm/numa.h>
>   #include <asm/page.h>
>   
>   #include <public/sysctl.h>
> @@ -256,10 +255,14 @@ static PAGE_LIST_HEAD(page_broken_list);
>    * BOOT-TIME ALLOCATOR
>    */
>   
> +#ifndef CONFIG_NUMA
>   /*
>    * first_valid_mfn is exported because it is use in ARM specific NUMA
>    * helpers. See comment in arch/arm/include/asm/numa.h.
>    */
> +#else
> +static
> +#endif
... this sort ugliness.

I am not going to Nack the patch. But I am definitely not going to ack 
it. with this change.
Cheers,

--
Julien Grall