Re: [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

[Vijay Kilari <vijay.kilari@gmail.com>]

On Wed, Jul 19, 2017 at 10:48 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 19/07/17 07:40, Vijay Kilari wrote:
>>
>> On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>>>
>>> On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kilari@gmail.com wrote:
>>>>
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> Add accessors for nodes[] and other static variables and
>>>> use those accessors. These variables are later accessed
>>>> outside the file when the code made generic in later
>>>> patches. However the coding style is not changed.
>>>>
>>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>> ---
>>>> v3: - Changed accessors parameter from int to unsigned int
>>>>     - Updated commit message
>>>>     - Fixed wrong indentation
>>>> ---
>>>>  xen/arch/x86/srat.c | 106
>>>> +++++++++++++++++++++++++++++++++++++++-------------
>>>>  1 file changed, 81 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>>>> index 535c9d7..42cca5a 100644
>>>> --- a/xen/arch/x86/srat.c
>>>> +++ b/xen/arch/x86/srat.c
>>>> @@ -41,6 +41,44 @@ static struct node
>>>> node_memblk_range[NR_NODE_MEMBLKS];
>>>>  static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>>>>  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
>>>>
>>>> +static struct node *get_numa_node(unsigned int id)
>>>> +{
>>>> +     return &nodes[id];
>>>> +}
>>>> +
>>>> +static nodeid_t get_memblk_nodeid(unsigned int id)
>>>> +{
>>>> +     return memblk_nodeid[id];
>>>> +}
>>>> +
>>>> +static nodeid_t *get_memblk_nodeid_map(void)
>>>> +{
>>>> +     return &memblk_nodeid[0];
>>>> +}
>>>> +
>>>> +static struct node *get_node_memblk_range(unsigned int memblk)
>>>> +{
>>>> +     return &node_memblk_range[memblk];
>>>> +}
>>>> +
>>>> +static int get_num_node_memblks(void)
>>>> +{
>>>> +     return num_node_memblks;
>>>> +}
>>>
>>>
>>> They should all be inline functions. And maybe at once lift to a header
>>> and add proper prefix since you mention they are going to be used later.
>>
>>
>> Currently these are static variables in x86/srat.c file.
>> In patch #9 I move them to common/numa.c file and make these functions
>> non-static.
>>
>> If I lift them to header file and make inline, then I have to make these
>> as
>> global variables.
>
>
> As I said on v2, I am not sure to understand the usefulness of those
> accessors over global variables...

These are static variables. To access across other files (arch specific)
these accessors are added. I have to make them global variables to use
outside of this file.

I am happy to make them global and make these accessors static inline
as suggested by Wei.

>
> You don't have any kind of sanity check, so they would do exactly the same
> job. The global variables would avoid so much churn.
>
> More that you tend to sometimes use global and other time static helpers...
>
> Cheers,
>
> --
> Julien Grall

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