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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2023年1月11日 0:38
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 02/17] xen/arm: implement helpers to get and update
> NUMA status
> 
> 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;
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?

> > +    DT_NUMA_ON,
> > +    DT_NUMA_OFF,
> > +};
> 
> ... the other two are also used only in a single file, at which point
> their placing in a header is also questionable.
>

This is a good point, I will move them to that file.

> > --- /dev/null
> > +++ b/xen/arch/arm/numa.c
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Arm Architecture support layer for NUMA.
> > + *
> > + * Copyright (C) 2021 Arm Ltd
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +#include <xen/init.h>
> > +#include <xen/numa.h>
> > +
> > +static enum dt_numa_status __read_mostly device_tree_numa;
> 
> __ro_after_init?
>

OK.
 
> > --- 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"?

Cheers,
Wei Chen
 
> Jan