On 25/04/17 13:20, Vijay Kilari wrote:
On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall <julien.gr...@arm.com> wrote:
Hello Vijay,
On 25/04/17 07:54, Vijay Kilari wrote:
On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <julien.gr...@arm.com>
wrote:
Hi Vijay,
On 28/03/17 16:53, vijay.kil...@gmail.com wrote:
From: Vijaya Kumar K <vijaya.ku...@cavium.com>
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@lists.xen.org
https://lists.xen.org/xen-devel
