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

[Julien Grall <julien@xxxxxxx>]

On 26/08/2021 08:24, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: 2021年8月26日 0:58
> > To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> > Subject: Re: [XEN RFC PATCH 26/40] xen/arm: Add boot and secondary CPU to
> > NUMA system
> >
> > Hi Wei,
> >
> > On 11/08/2021 11:24, Wei Chen wrote:
> > > When cpu boot up, we have add them to NUMA system. In current
> > > stage, we have not parsed the NUMA data, but we have created
> > > a fake NUMA node. So, in this patch, all CPU will be added
> > > to NUMA node#0. After the NUMA data has been parsed from device
> > > tree, the CPU will be added to correct NUMA node as the NUMA
> > > data described.
> > >
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > >    xen/arch/arm/setup.c       | 6 ++++++
> > >    xen/arch/arm/smpboot.c     | 6 ++++++
> > >    xen/include/asm-arm/numa.h | 1 +
> > >    3 files changed, 13 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index 3c58d2d441..7531989f21 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -918,6 +918,12 @@ void __init start_xen(unsigned long
> > boot_phys_offset,
> > >        processor_id();
> > >
> > > +    /*
> > > +     * If Xen is running on a NUMA off system, there will
> > > +     * be a node#0 at least.
> > > +     */
> > > +    numa_add_cpu(0);
> > > +
> > >        smp_init_cpus();
> > >        cpus = smp_get_max_cpus();
> > >        printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", cpus);
> > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > > index a1ee3146ef..aa78958c07 100644
> > > --- 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).
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.
If you think the current position on x86 is not correct, then it should 
be changed at as well. However, I don't know the story behind the 
position of the call on x86. You may want to ask the x86 maintainers.
Cheers,

--
Julien Grall