[7/8] mm/migrate.c: move page on next iteration
diff mbox series

Message ID 20200119030636.11899-8-richardw.yang@linux.intel.com
State New
Headers show
Series
  • mm/migrate.c: cleanup on do_pages_move()
Related show

Commit Message

Wei Yang Jan. 19, 2020, 3:06 a.m. UTC
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.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 mm/migrate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Michal Hocko Jan. 20, 2020, 10:02 a.m. UTC | #1
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.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  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
>
Wei Yang Jan. 21, 2020, 1:22 a.m. UTC | #2
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@linux.intel.com>
>> ---
>>  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
Michal Hocko Jan. 21, 2020, 8:43 a.m. UTC | #3
On Tue 21-01-20 09:22:00, Wei Yang wrote:
> 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.

i is the main iterator to process the whole imput. start is another one
to control the batch to migrate. We do not need/want more. More clear?
Wei Yang Jan. 22, 2020, 12:40 a.m. UTC | #4
On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote:
>On Tue 21-01-20 09:22:00, Wei Yang wrote:
>> 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.
>
>i is the main iterator to process the whole imput. start is another one
>to control the batch to migrate. We do not need/want more. More clear?

Yes, more clear.

I hope you are angry with me. Sorry for my poor English.

>-- 
>Michal Hocko
>SUSE Labs
Michal Hocko Jan. 22, 2020, 8:17 a.m. UTC | #5
On Wed 22-01-20 08:40:12, Wei Yang wrote:
> On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote:
> >On Tue 21-01-20 09:22:00, Wei Yang wrote:
> >> 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.
> >
> >i is the main iterator to process the whole imput. start is another one
> >to control the batch to migrate. We do not need/want more. More clear?
> 
> Yes, more clear.

Great

> I hope you are angry with me. Sorry for my poor English.

Heh, not at all.

Patch
diff mbox series

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: