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
