RE: [XEN RFC PATCH 27/40] xen/arm: build CPU NUMA node map while creating cpu_logical_map

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月26日 1:07
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>
> Subject: Re: [XEN RFC PATCH 27/40] xen/arm: build CPU NUMA node map while
> creating cpu_logical_map
> 
> Hi Wei,
> 
> On 11/08/2021 11:24, Wei Chen wrote:
> > Sometimes, CPU logical ID maybe different with physical CPU ID.
> > Xen is using CPU logial ID for runtime usage, so we should use
> > CPU logical ID to create map between NUMA node and CPU.
> 
> This commit message gives the impression that you are trying to fix a
> bug. However, what you are explaining is the reason why the code will
> use the logical ID rather than physical ID.
> 
> I think the commit message should explain what the patch is doing. You
> can then add an explanation why you are using the CPU logical ID.
> Something like "Note we storing the CPU logical ID because...".
> 
> 

Ok

> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/smpboot.c | 31 ++++++++++++++++++++++++++++++-
> >   1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index aa78958c07..dd5a45bffc 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -121,7 +121,12 @@ static void __init dt_smp_init_cpus(void)
> >       {
> >           [0 ... NR_CPUS - 1] = MPIDR_INVALID
> >       };
> > +    static nodeid_t node_map[NR_CPUS] __initdata =
> > +    {
> > +        [0 ... NR_CPUS - 1] = NUMA_NO_NODE
> > +    };
> >       bool bootcpu_valid = false;
> > +    uint32_t nid = 0;
> >       int rc;
> >
> >       mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
> > @@ -172,6 +177,26 @@ static void __init dt_smp_init_cpus(void)
> >               continue;
> >           }
> >
> > +#ifdef CONFIG_DEVICE_TREE_NUMA
> > +        /*
> > +         *  When CONFIG_DEVICE_TREE_NUMA is set, try to fetch numa
> infomation
> > +         * from CPU dts node, otherwise the nid is always 0.
> > +         */
> > +        if ( !dt_property_read_u32(cpu, "numa-node-id", &nid) )
> 
> You can avoid the #ifdef by writing:
> 
> if ( IS_ENABLED(CONFIG_DEVICE_TREE_NUMA) && ... )
> 
> However, I would using CONFIG_NUMA because this code is already DT
> specific. So we can shorten the name a bit.
> 

OK

> > +        {
> > +            printk(XENLOG_WARNING
> > +                "cpu[%d] dts path: %s: doesn't have numa infomation!\n",
> 
> s/information/information/

OK

> 
> > +                cpuidx, dt_node_full_name(cpu));
> > +            /*
> > +             * The the early stage of NUMA initialization, when Xen
> found any
> 
> s/The/During/?

Oh, yes, I will fix it.

> 
> > +             * CPU dts node doesn't have numa-node-id info, the NUMA
> will be
> > +             * treated as off, all CPU will be set to a FAKE node 0. So
> if we
> > +             * get numa-node-id failed here, we should set nid to 0.
> > +             */
> > +            nid = 0;
> > +        }
> > +#endif
> > +
> >           /*
> >            * 8 MSBs must be set to 0 in the DT since the reg property
> >            * defines the MPIDR[23:0]
> > @@ -231,9 +256,12 @@ static void __init dt_smp_init_cpus(void)
> >           {
> >               printk("cpu%d init failed (hwid %"PRIregister"): %d\n", i,
> hwid, rc);
> >               tmp_map[i] = MPIDR_INVALID;
> > +            node_map[i] = NUMA_NO_NODE;
> >           }
> > -        else
> > +        else {
> >               tmp_map[i] = hwid;
> > +            node_map[i] = nid;
> > +        }
> >       }
> >
> >       if ( !bootcpu_valid )
> > @@ -249,6 +277,7 @@ static void __init dt_smp_init_cpus(void)
> >               continue;
> >           cpumask_set_cpu(i, &cpu_possible_map);
> >           cpu_logical_map(i) = tmp_map[i];
> > +        numa_set_node(i, node_map[i]);
> >       }
> >   }
> >    >
> 
> Cheers,
> 
> --
> Julien Grall