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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

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