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: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.

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

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

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

I am ok if it helps in review.

>
>
>>
>>>
>>>
>>>>  #endif /* _XEN_NUMA_H */
>>>>
>>>
>>> --
>>> Julien Grall
>
>
> --
> Julien Grall

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