Re: [PATCH v2 2/2] mm: migrate: Allocate the node_demotion structure dynamically

[Baolin Wang]

On 2021/11/11 16:51, Huang, Ying wrote:
> Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes:
>
> > For the worst case (MAX_NUMNODES=1024), the node_demotion structure can
> > consume 32k bytes, which appears too large, so we can change to allocate
> > node_demotion dynamically at initialization time. Meanwhile allocating
> > the target demotion nodes array dynamically to select a suitable size
> > according to the MAX_NUMNODES.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > ---
> >   mm/migrate.c | 38 +++++++++++++++++++++++++++++---------
> >   1 file changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 126e9e6..0145b38 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1152,10 +1152,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> >   #define DEFAULT_DEMOTION_TARGET_NODES 15
> >   struct demotion_nodes {
> >   	unsigned short nr;
> > -	short nodes[DEFAULT_DEMOTION_TARGET_NODES];
> > +	short nodes[];
> >   };
> >   
> > -static struct demotion_nodes node_demotion[MAX_NUMNODES] __read_mostly;
> > +static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly;
> > +static unsigned short target_nodes_max;
>
> I think we can use something as below,
>
>    #if MAX_NUMNODES < DEFAULT_DEMOTION_TARGET_NODES
>    #define DEMOTION_TARGET_NODES   (MAX_NUMNODES - 1)
>    #else
>    #define DEMOTION_TARGET_NODES   DEFAULT_DEMOTION_TARGET_NODES
>    #endif

Yes, looks better.

>    static struct demotion_nodes *node_demotion;
>
> Then we can allocate nr_node_ids * sizeof(struct demotion_nodes) for node_demotion.

Yeah, this is simple. The reason I want to declare the structure like 
"struct demotion_nodes *node_demotion[MAX_NUMNODES]" is that, we can 
validate the non-possible nodes which are invalid to demote memory, and 
in case the node_demotion[nid] is failed to be allocated which can be 
validated, though this is unlikely. However, I agree with you to keep 
things simple now and can be merged into patch 1. Will do in next 
version. Thanks.