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

[Julien Grall <julien.grall@xxxxxxx>]

On 20/07/17 08:41, Vijay Kilari wrote:
> On Wed, Jul 19, 2017 at 10:48 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > On 19/07/17 07:40, Vijay Kilari wrote:
> > > On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > > > On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kilari@xxxxxxxxx wrote:
> > > > > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > > 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.
I don't think the helpers are useful... They just return a value without 
any sanity check. You can directly use the global variable in the code.
I would much prefer if you use plain global.

Cheers,

--
Julien Grall

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