[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 04/25] x86: NUMA: Add accessors for acpi_numa, numa_off and numa_fake variables
On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <julien.grall@xxxxxxx> wrote: > > > 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. IMO, In x86 -1 means disabled 0 enabled but not numa initialized 1 enabled and numa initialized. I clubbed 0 & 1. I was referring to below code in x86 where in acpi_numa = 0 means srat_disabled() returns false. Which means acpi_numa is enabled implicitly. int srat_disabled(void) { return numa_off || acpi_numa < 0; } When I changed acpi_numa to bool, it is initialized to 1 and changed below code. bool srat_disabled(void) { return numa_off || acpi_numa == 0; } Also this srat_disabed() is used in acpi_numa_memory_affinity_init which is called from acpi_numa_init() before calling acpi_scan_nodes(). > >> >>> >>>> >>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |