RE: [PATCH v4 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年9月8日 19:42
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 2/6] xen/x86: move generically usable NUMA code
> from x86 to common
> 
> On 08.09.2022 12:32, Wei Chen wrote:
> > On 2022/9/8 17:09, Jan Beulich wrote:
> >> On 02.09.2022 05:31, Wei Chen wrote:
> >>> --- /dev/null
> >>> +++ b/xen/common/numa.c
> >>> @@ -0,0 +1,442 @@
> >>> +/*
> >>> + * Generic VM initialization for NUMA setups.
> >>> + * Copyright 2002,2003 Andi Kleen, SuSE Labs.
> >>> + * Adapted for Xen: Ryan Harper <ryanh@xxxxxxxxxx>
> >>> + */
> >>> +
> >>> +#include <xen/init.h>
> >>> +#include <xen/keyhandler.h>
> >>> +#include <xen/mm.h>
> >>> +#include <xen/nodemask.h>
> >>> +#include <xen/numa.h>
> >>> +#include <xen/param.h>
> >>> +#include <xen/sched.h>
> >>> +#include <xen/softirq.h>
> >>> +
> >>> +struct node_data __ro_after_init node_data[MAX_NUMNODES];
> >>> +
> >>> +/* Mapping from pdx to node id */
> >>> +unsigned int __ro_after_init memnode_shift;
> >>> +unsigned long __ro_after_init memnodemapsize;
> >>> +uint8_t *__ro_after_init memnodemap;
> >>> +static uint8_t __ro_after_init _memnodemap[64];
> >>
> >> These last two want to use nodeid_t instead of uint8_t. Originally
> >> the latter used typeof(*memnodemap) for (I think - iirc it was me who
> >> made it that way) this reason: That way correcting memnodemap's type
> >> would have propagated without the need for further adjustments.
> >>
> >
> > Thanks for this info, should I need to restore it to use
> > "typeof(*memnodemap)" in next version ?
> 
> That would be more in line with the original code, but it's not
> strictly necessary once nodeid_t if properly used for these variables.
> I'd leave it up to you as long as you switch to nodeid_t.
> 

Ok, I will think more about it in next version.

> >>> +nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
> >>> +    [0 ... NR_CPUS-1] = NUMA_NO_NODE
> >>> +};
> >>> +
> >>> +cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
> >>> +
> >>> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> >>> +
> >>> +bool __read_mostly numa_off;
> >>
> >> The v3 review discussing this possibly becoming __ro_after_init didn't
> >> really finish (you didn't reply to my latest request there), but you
> >> also didn't change the attribute. Please clarify.
> >>
> >
> > I think I had answered your question by:
> >  >> I think yes, it will be used in numa_disabled and numa_disabled will
> >  >> be called in cpu_add."
> >
> > And you replied me with:
> >  > In the original code I cannot spot such a path - can you please point
> >  > out how exactly you see numa_disabled() reachable from cpu_add()? I'm
> >  > clearly overlooking something ..."
> >
> > But there is a time difference here, your reply was sent after I sent
> > v3, maybe you didn't notice it
> 
> Which suggests you might better have waited with sending v3 until the
> discussion had settled.
> 
> > About the new question:
> > cpu_add will call srat_disabled, srat_disabled will access numa_off.
> > srat_disabled is a function without __init.
> 
> But the request wasn't to make the variable __initdata. That would be
> wrong of course. Since srat_disabled() only reads numa_off,
> __ro_after_init does look usable to me.
> 

Oh, yes, you're right. I had thought wrong. I will correct this.

Cheers,
Wei Chen.

> Jan