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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2021/11/11 16:51, Huang, Ying wrote:
> Baolin Wang <baolin.wang@linux.alibaba.com> 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@linux.alibaba.com>
>> ---
>>   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.