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 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...
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel