Re: [PATCH 7/8] mm/migrate.c: move page on next iteration

[Wei Yang]

On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:35, Wei Yang wrote:
>> When page is not successfully queued for migration, we would move pages
>> on pagelist immediately. Actually, this could be done in the next
>> iteration by telling it we need some help.
>> 
>> This patch adds a new variable need_move to be an indication. After
>> this, we only need to call move_pages_and_store_status() twice.
>
>No! Not another iterator. There are two and this makes the function
>quite hard to follow already. We do not want to add a third one.
>

Two iterators here are? I don't get your point.

>> Signed-off-by: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
>> ---
>>  mm/migrate.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index aee5aeb082c4..2a857fec65b6 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  	LIST_HEAD(pagelist);
>>  	int start, i;
>>  	int err = 0, err1;
>> +	int need_move = 0;
>>  
>>  	migrate_prep();
>>  
>> @@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  		if (current_node == NUMA_NO_NODE) {
>>  			current_node = node;
>>  			start = i;
>> -		} else if (node != current_node) {
>> +		} else if (node != current_node || need_move) {
>>  			err = move_pages_and_store_status(mm, current_node,
>> -					&pagelist, status, start, i - start);
>> +					&pagelist, status, start,
>> +					i - start - need_move);
>>  			if (err)
>>  				goto out;
>>  			start = i;
>>  			current_node = node;
>> +			need_move = 0;
>>  		}
>>  
>>  		/*
>> @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  			continue;
>>  		}
>>  
>> +		/* Ask next iteration to move us */
>> +		need_move = 1;
>> +
>>  		/*
>>  		 * Two possible cases for err here:
>>  		 * == 0: page is already on the target node, then store
>> @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  		err = store_status(status, i, err ? : current_node, 1);
>>  		if (err)
>>  			goto out_flush;
>> -
>> -		err = move_pages_and_store_status(mm, current_node, &pagelist,
>> -				status, start, i - start);
>> -		if (err)
>> -			goto out;
>> -		current_node = NUMA_NO_NODE;
>>  	}
>>  out_flush:
>>  	/* Make sure we do not overwrite the existing error */
>>  	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
>> -				status, start, i - start);
>> +				status, start, i - start - need_move);
>>  	if (err >= 0)
>>  		err = err1;
>>  out:
>> -- 
>> 2.17.1
>> 
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me