RE: [PATCH 22/37] xen/arm: use NR_MEM_BANKS to override default NR_NODE_MEMBLKS

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年9月24日 9:35
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [PATCH 22/37] xen/arm: use NR_MEM_BANKS to override default
> NR_NODE_MEMBLKS
> 
> On Thu, 23 Sep 2021, Wei Chen wrote:
> > As a memory range described in device tree cannot be split across
> > multiple nodes. So we define NR_NODE_MEMBLKS as NR_MEM_BANKS in
> > arch header.
> 
> This statement is true but what is the goal of this patch? Is it to
> reduce code size and memory consumption?
> 

No, when Julien and I discussed this in last version[1], we hadn't thought
so deeply. We just thought a memory range described in DT cannot be split
across multiple nodes. So NR_MEM_BANKS should be equal to NR_MEM_BANKS.

https://lists.xenproject.org/archives/html/xen-devel/2021-08/msg00974.html 

> I am asking because NR_MEM_BANKS is 128 and
> NR_NODE_MEMBLKS=2*MAX_NUMNODES which is 64 by default so again
> NR_NODE_MEMBLKS is 128 before this patch.
> 
> In other words, this patch alone doesn't make any difference; at least
> doesn't make any difference unless CONFIG_NR_NUMA_NODES is increased.
> 
> So, is the goal to reduce memory usage when CONFIG_NR_NUMA_NODES is
> higher than 64?
> 

I also thought about this problem when I was writing this patch.
CONFIG_NR_NUMA_NODES is increasing, but NR_MEM_BANKS is a fixed
value, then NR_MEM_BANKS can be smaller than CONFIG_NR_NUMA_NODES
at one point.

But I agree with Julien's suggestion, NR_MEM_BANKS and NR_NODE_MEMBLKS
must be aware of each other. I had thought to add some ASSERT check,
but I don't know how to do it better. So I post this patch for more
suggestion.

> 
> > And keep default NR_NODE_MEMBLKS in common header
> > for those architectures NUMA is disabled.
> 
> This last sentence is not accurate: on x86 NUMA is enabled and
> NR_NODE_MEMBLKS is still defined in xen/include/xen/numa.h (there is no
> x86 definition of it)
> 

Yes.

> 
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >  xen/include/asm-arm/numa.h | 8 +++++++-
> >  xen/include/xen/numa.h     | 2 ++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > index 8f1c67e3eb..21569e634b 100644
> > --- a/xen/include/asm-arm/numa.h
> > +++ b/xen/include/asm-arm/numa.h
> > @@ -3,9 +3,15 @@
> >
> >  #include <xen/mm.h>
> >
> > +#include <asm/setup.h>
> > +
> >  typedef u8 nodeid_t;
> >
> > -#ifndef CONFIG_NUMA
> > +#ifdef CONFIG_NUMA
> > +
> > +#define NR_NODE_MEMBLKS NR_MEM_BANKS
> > +
> > +#else
> >
> >  /* Fake one node for now. See also node_online_map. */
> >  #define cpu_to_node(cpu) 0
> > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> > index 1978e2be1b..1731e1cc6b 100644
> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -12,7 +12,9 @@
> >  #define MAX_NUMNODES    1
> >  #endif
> >
> > +#ifndef NR_NODE_MEMBLKS
> >  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
> > +#endif
> >
> >  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
> >
> > --
> > 2.25.1
> >