Re: [PATCH v2 02/17] xen/arm: implement helpers to get and update NUMA status

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

On 2023/1/12 16:08, Jan Beulich wrote:
> On 12.01.2023 07:22, Wei Chen wrote:
> > > -----Original Message-----
> > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > Sent: 2023年1月11日 0:38
> > >
> > > On 10.01.2023 09:49, Wei Chen wrote:
> > > > --- a/xen/arch/arm/include/asm/numa.h
> > > > +++ b/xen/arch/arm/include/asm/numa.h
> > > > @@ -22,6 +22,12 @@ typedef u8 nodeid_t;
> > > >    */
> > > >   #define NR_NODE_MEMBLKS NR_MEM_BANKS
> > > >
> > > > +enum dt_numa_status {
> > > > +    DT_NUMA_INIT,
> > > I don't see any use of this. I also think the name isn't good, as INIT
> > > can be taken for "initializer" as well as "initialized". Suggesting an
> > > alternative would require knowing what the future plans with this are;
> > > right now ...
> > static enum dt_numa_status __read_mostly device_tree_numa;
> There's no DT_NUMA_INIT here. You _imply_ it having a value of zero.
How about I assign device_tree_numa explicitly like:
... __read_mostly device_tree_numa = DT_NUMA_UNINIT;

> > We use DT_NUMA_INIT to indicate device_tree_numa is in a default value
> > (System's initial value, hasn't done initialization). Maybe rename it
> > To DT_NUMA_UNINIT be better?
> Perhaps, yes.
>
> > > > --- a/xen/arch/x86/include/asm/numa.h
> > > > +++ b/xen/arch/x86/include/asm/numa.h
> > > > @@ -12,7 +12,6 @@ extern unsigned int numa_node_to_arch_nid(nodeid_t n);
> > > >
> > > >   #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
> > > >
> > > > -extern bool numa_disabled(void);
> > > >   extern nodeid_t setup_node(unsigned int pxm);
> > > >   extern void srat_detect_node(int cpu);
> > > >
> > > > --- a/xen/include/xen/numa.h
> > > > +++ b/xen/include/xen/numa.h
> > > > @@ -55,6 +55,7 @@ extern void numa_init_array(void);
> > > >   extern void numa_set_node(unsigned int cpu, nodeid_t node);
> > > >   extern void numa_initmem_init(unsigned long start_pfn, unsigned long
> > > end_pfn);
> > > >   extern void numa_fw_bad(void);
> > > > +extern bool numa_disabled(void);
> > > >
> > > >   extern int arch_numa_setup(const char *opt);
> > > >   extern bool arch_numa_unavailable(void);
> > > How is this movement of a declaration related to the subject of the patch?
> > Can I add some messages in commit log to say something like "As we have
> > Implemented numa_disabled for Arm, so we move numa_disabled to common header"?
> See your own patch 3, where you have a similar statement (albeit you mean
> "declaration" there, not "definition"). However, right now numa_disabled()
> is a #define on Arm, so the declaration becoming common isn't really
> warranted. In fact it'll get in the way of converting function-like macros
> to inline functions for Misra.
Yes, I think you're right. This may also seem a little strange,when we 
disable NUMA, there are two headers have numa_disabled statement. I will 
revert this change. And I also will covert the macro to a static inline 
function.
Cheers,
Wei Chen

> Jan