RE: [XEN RFC PATCH 26/40] xen/arm: Add boot and secondary CPU to NUMA system

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan, Julien,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2021年8月26日 17:40
> To: Julien Grall <julien@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx
> Subject: Re: [XEN RFC PATCH 26/40] xen/arm: Add boot and secondary CPU to
> NUMA system
> 
> On 26.08.2021 10:49, Julien Grall wrote:
> > On 26/08/2021 08:24, Wei Chen wrote:
> >>> -----Original Message-----
> >>> From: Julien Grall <julien@xxxxxxx>
> >>> Sent: 2021年8月26日 0:58
> >>> On 11/08/2021 11:24, Wei Chen wrote:
> >>>> --- a/xen/arch/arm/smpboot.c
> >>>> +++ b/xen/arch/arm/smpboot.c
> >>>> @@ -358,6 +358,12 @@ void start_secondary(void)
> >>>>         */
> >>>>        smp_wmb();
> >>>>
> >>>> +    /*
> >>>> +     * If Xen is running on a NUMA off system, there will
> >>>> +     * be a node#0 at least.
> >>>> +     */
> >>>> +    numa_add_cpu(cpuid);
> >>>> +
> >>>
> >>> On x86, numa_add_cpu() will be called before the pCPU is brought up. I
> >>> am not quite too sure why we are doing it differently here. Can you
> >>> clarify it?
> >>
> >> Of course we can invoke numa_add_cpu before cpu_up as x86. But in my
> tests,
> >> I found when cpu bring up failed, this cpu still be add to NUMA.
> Although
> >> this does not affect the execution of the code (because CPU is offline),
> >> But I don't think adding a offline CPU to NUMA makes sense.
> >
> > Right, but again, why do you want to solve the problem on Arm and not
> > x86? After all, NUMA is not architecture specific (in fact you move most
> > of the code in common).
> >

I am not very familiar with x86, so when I was composing this patch series,
I always thought that if I could solve it inside Arm Arch, I would solve it
inside Arm Arch. That seems a bit conservative, and inappropriate on solving
this problem.

> > In fact, the risk, is someone may read arch/x86 and doesn't realize the
> > CPU is not in the node until late on Arm.
> >
> > So I think we should call numa_add_cpu() around the same place on all
> > the architectures.
> 
> FWIW: +1

I agree. As Jan in this discussion. How about following current x86's
numa_add_cpu behaviors in __start_xen, but add some code to revert
numa_add_cpu when cpu_up failed (both Arm and x86)?

> 
> Jan