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>]

On 25/04/17 13:20, Vijay Kilari wrote:
> On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > 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.
> Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use of
> having 3 values to say enable or disable.
Then explain why in the commit message and don't let people discover. If 
you have not done it, I would recommend to read:
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

> > > 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 = 0 means it is enabled by default on x86.
In acpi_scan_nodes:

if (acpi_numa <= 0)
  return -1;

So it does not seem that 0 means enabled.

> > > 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?
> I agree with making inline. But I don't think there is any harm in making them
> static and adding helpers.
But why? Why don't you keep the code as it is? You modify code without 
any justification and not for the better.
Cheers,

--
Julien Grall

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