Re: [Xen-devel] [RFC PATCH v2 04/25] x86: NUMA: Add accessors for acpi_numa, numa_off and numa_fake variables

[Julien Grall <julien.grall@xxxxxxx>]

Hello Vijay,

On 25/04/17 07:54, Vijay Kilari wrote:
> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Vijay,
> >
> > On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
> > > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > >
> > > Add accessor functions for acpi_numa and numa_off static
> > > variables. Init value of acpi_numa is set 1 instead of 0.
> >
> > Please explain why you change the acpi_numa value from 0 to 1.
> previously acpi_numa was s8 and are using 0 and -1 values. I have made
> it bool and set
> the initial value to 1.
Are you sure? With a quick grep I spot it sounds like acpi_numa can have 
the value 0, -1, 1.
> By setting 1, we are enabling acpi_numa by default. If not enabled, the below
> call has check srat_disabled() before proceeding fails.
My understanding is on x86 acpi_numa is disabled by default and will be 
enabled if they are able to parse the SRAT. So why are you changing the 
behavior for x86?
> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
> {
> ....
>
>     if ( srat_disabled() )
>         return;
>
> }
>
> > Also, I am not sure to understand the benefits of those helpers. Why do you
> > need them? Why not using the global variable directly, this will avoid to
> > call a function just for returning a value...
> These helpers are used by both common code and arch specific code later.
> Hence made these global variables as static and added helpers
The variables were global so they could already be used anywhere.

Furthermore, those helpers are not even inlined, so everytime you want 
to access read acpi_numa you have to do a branch then a memory access.
So what is the point to do that?

> > > diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
> > > index a766688..9298d42 100644
> > > --- a/xen/include/asm-x86/acpi.h
> > > +++ b/xen/include/asm-x86/acpi.h
> > > @@ -103,7 +103,6 @@ extern void acpi_reserve_bootmem(void);
> > >
> > >  #define ARCH_HAS_POWER_INIT    1
> > >
> > > -extern s8 acpi_numa;
> > >  extern int acpi_scan_nodes(u64 start, u64 end);
> > >  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
> > >
> > > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> > > index bb22bff..ae5768b 100644
> > > --- a/xen/include/asm-x86/numa.h
> > > +++ b/xen/include/asm-x86/numa.h
> > > @@ -30,10 +30,7 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
> > >
> > >  extern void numa_add_cpu(int cpu);
> > >  extern void numa_init_array(void);
> > > -extern bool_t numa_off;
> > > -
> > > -
> > > -extern int srat_disabled(void);
> > > +extern bool srat_disabled(void);
> > >  extern void numa_set_node(int cpu, nodeid_t node);
> > >  extern nodeid_t setup_node(unsigned int pxm);
> > >  extern void srat_detect_node(int cpu);
> > > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> > > index 7aef1a8..7f6d090 100644
> > > --- a/xen/include/xen/numa.h
> > > +++ b/xen/include/xen/numa.h
> > > @@ -18,4 +18,7 @@
> > >    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
> > >     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
> > >
> > > +bool is_numa_off(void);
> > > +bool get_acpi_numa(void);
> > > +void set_acpi_numa(bool val);
> >
> > I am not sure to understand why you add those helpers directly here in
> > xen/numa.h. IHMO, This should belong to the moving code patches.
> To have code moving patches doing only code movement, I have exported
> in the patch it is defined.
Sorry but what are prototypes if not code?

The point of moving the prototypes in the patch which move the actual 
declarations is helping the reviewers to check if you correctly moved 
everything.
> > >  #endif /* _XEN_NUMA_H */
> > --
> > Julien Grall
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel