[PATCH 0/8] mm/migrate.c: cleanup on do_pages_move()

[PATCH 0/8] mm/migrate.c: cleanup on do_pages_move()

[Wei Yang]

The logic in do_pages_move() is a little mess for audience to read and has
some potential error on handling the return value. Especially there are
three calls on do_move_pages_to_node() and store_status() with almost the
same form.

This patch set tries to make the code a little friendly for audience by
consolidate the calls and remove some unnecessary repeat code.

After this, we can do a better fix.

Wei Yang (8):
  mm/migrate.c: skip node check if done in last round
  mm/migrate.c: not necessary to check start and i
  mm/migrate.c: reform the last call on do_move_pages_to_node()
  mm/migrate.c: wrap do_move_pages_to_node() and store_status()
  mm/migrate.c: check pagelist in move_pages_and_store_status()
  mm/migrate.c: handle same node and add failure in the same way
  mm/migrate.c: move page on next iteration
  mm/migrate.c: use break instead of goto out_flush

 mm/migrate.c | 90 ++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 42 deletions(-)

-- 
2.17.1

[PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Wei Yang]

Before move page to target node, we would check if the node id is valid.
In case we would try to move pages to the same target node, it is not
necessary to do the check each time.

This patch tries to skip the check if the node has been checked.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index 430fdccc733e..ba7cf4fa43a0 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			goto out_flush;
 		addr = (unsigned long)untagged_addr(p);
 
-		err = -ENODEV;
-		if (node < 0 || node >= MAX_NUMNODES)
-			goto out_flush;
-		if (!node_state(node, N_MEMORY))
-			goto out_flush;
+		/* Check node if it is not checked. */
+		if (current_node == NUMA_NO_NODE || node != current_node) {
+			err = -ENODEV;
+			if (node < 0 || node >= MAX_NUMNODES)
+				goto out_flush;
+			if (!node_state(node, N_MEMORY))
+				goto out_flush;
 
-		err = -EACCES;
-		if (!node_isset(node, task_nodes))
-			goto out_flush;
+			err = -EACCES;
+			if (!node_isset(node, task_nodes))
+				goto out_flush;
+		}
 
 		if (current_node == NUMA_NO_NODE) {
 			current_node = node;
-- 
2.17.1

Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Michal Hocko]

On Sun 19-01-20 11:06:29, Wei Yang wrote:
> Before move page to target node, we would check if the node id is valid.
> In case we would try to move pages to the same target node, it is not
> necessary to do the check each time.
> 
> This patch tries to skip the check if the node has been checked.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/migrate.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 430fdccc733e..ba7cf4fa43a0 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  			goto out_flush;
>  		addr = (unsigned long)untagged_addr(p);
>  
> -		err = -ENODEV;
> -		if (node < 0 || node >= MAX_NUMNODES)
> -			goto out_flush;
> -		if (!node_state(node, N_MEMORY))
> -			goto out_flush;
> +		/* Check node if it is not checked. */
> +		if (current_node == NUMA_NO_NODE || node != current_node) {
> +			err = -ENODEV;
> +			if (node < 0 || node >= MAX_NUMNODES)
> +				goto out_flush;
> +			if (!node_state(node, N_MEMORY))
> +				goto out_flush;

This makes the code harder to read IMHO. The original code checks the
valid node first and it doesn't conflate that with the node caching
logic which your change does.

>  
> -		err = -EACCES;
> -		if (!node_isset(node, task_nodes))
> -			goto out_flush;
> +			err = -EACCES;
> +			if (!node_isset(node, task_nodes))
> +				goto out_flush;
> +		}
>  
>  		if (current_node == NUMA_NO_NODE) {
>  			current_node = node;
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Wei Yang]

On Mon, Jan 20, 2020 at 10:36:46AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:29, Wei Yang wrote:
>> Before move page to target node, we would check if the node id is valid.
>> In case we would try to move pages to the same target node, it is not
>> necessary to do the check each time.
>> 
>> This patch tries to skip the check if the node has been checked.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  mm/migrate.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>> 
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 430fdccc733e..ba7cf4fa43a0 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  			goto out_flush;
>>  		addr = (unsigned long)untagged_addr(p);
>>  
>> -		err = -ENODEV;
>> -		if (node < 0 || node >= MAX_NUMNODES)
>> -			goto out_flush;
>> -		if (!node_state(node, N_MEMORY))
>> -			goto out_flush;
>> +		/* Check node if it is not checked. */
>> +		if (current_node == NUMA_NO_NODE || node != current_node) {
>> +			err = -ENODEV;
>> +			if (node < 0 || node >= MAX_NUMNODES)
>> +				goto out_flush;
>> +			if (!node_state(node, N_MEMORY))
>> +				goto out_flush;
>
>This makes the code harder to read IMHO. The original code checks the
>valid node first and it doesn't conflate that with the node caching
>logic which your change does.
>

I am sorry, would you mind showing me an example about the conflate in my
change? I don't get it.

>>  
>> -		err = -EACCES;
>> -		if (!node_isset(node, task_nodes))
>> -			goto out_flush;
>> +			err = -EACCES;
>> +			if (!node_isset(node, task_nodes))
>> +				goto out_flush;
>> +		}
>>  
>>  		if (current_node == NUMA_NO_NODE) {
>>  			current_node = node;
>> -- 
>> 2.17.1
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Michal Hocko]

On Tue 21-01-20 06:25:40, Wei Yang wrote:
> On Mon, Jan 20, 2020 at 10:36:46AM +0100, Michal Hocko wrote:
> >On Sun 19-01-20 11:06:29, Wei Yang wrote:
> >> Before move page to target node, we would check if the node id is valid.
> >> In case we would try to move pages to the same target node, it is not
> >> necessary to do the check each time.
> >> 
> >> This patch tries to skip the check if the node has been checked.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> ---
> >>  mm/migrate.c | 19 +++++++++++--------
> >>  1 file changed, 11 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index 430fdccc733e..ba7cf4fa43a0 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >>  			goto out_flush;
> >>  		addr = (unsigned long)untagged_addr(p);
> >>  
> >> -		err = -ENODEV;
> >> -		if (node < 0 || node >= MAX_NUMNODES)
> >> -			goto out_flush;
> >> -		if (!node_state(node, N_MEMORY))
> >> -			goto out_flush;
> >> +		/* Check node if it is not checked. */
> >> +		if (current_node == NUMA_NO_NODE || node != current_node) {
> >> +			err = -ENODEV;
> >> +			if (node < 0 || node >= MAX_NUMNODES)
> >> +				goto out_flush;
> >> +			if (!node_state(node, N_MEMORY))
> >> +				goto out_flush;
> >
> >This makes the code harder to read IMHO. The original code checks the
> >valid node first and it doesn't conflate that with the node caching
> >logic which your change does.
> >
> 
> I am sorry, would you mind showing me an example about the conflate in my
> change? I don't get it.

NUMA_NO_NODE is the iteration logic, right? It resets the batching node.
Node check read from the userspace is an input sanitization. Do not put
those two into the same checks. More clear now?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Wei Yang]

On Tue, Jan 21, 2020 at 09:42:05AM +0100, Michal Hocko wrote:
>On Tue 21-01-20 06:25:40, Wei Yang wrote:
>> On Mon, Jan 20, 2020 at 10:36:46AM +0100, Michal Hocko wrote:
>> >On Sun 19-01-20 11:06:29, Wei Yang wrote:
>> >> Before move page to target node, we would check if the node id is valid.
>> >> In case we would try to move pages to the same target node, it is not
>> >> necessary to do the check each time.
>> >> 
>> >> This patch tries to skip the check if the node has been checked.
>> >> 
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> ---
>> >>  mm/migrate.c | 19 +++++++++++--------
>> >>  1 file changed, 11 insertions(+), 8 deletions(-)
>> >> 
>> >> diff --git a/mm/migrate.c b/mm/migrate.c
>> >> index 430fdccc733e..ba7cf4fa43a0 100644
>> >> --- a/mm/migrate.c
>> >> +++ b/mm/migrate.c
>> >> @@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>> >>  			goto out_flush;
>> >>  		addr = (unsigned long)untagged_addr(p);
>> >>  
>> >> -		err = -ENODEV;
>> >> -		if (node < 0 || node >= MAX_NUMNODES)
>> >> -			goto out_flush;
>> >> -		if (!node_state(node, N_MEMORY))
>> >> -			goto out_flush;
>> >> +		/* Check node if it is not checked. */
>> >> +		if (current_node == NUMA_NO_NODE || node != current_node) {
>> >> +			err = -ENODEV;
>> >> +			if (node < 0 || node >= MAX_NUMNODES)
>> >> +				goto out_flush;
>> >> +			if (!node_state(node, N_MEMORY))
>> >> +				goto out_flush;
>> >
>> >This makes the code harder to read IMHO. The original code checks the
>> >valid node first and it doesn't conflate that with the node caching
>> >logic which your change does.
>> >
>> 
>> I am sorry, would you mind showing me an example about the conflate in my
>> change? I don't get it.
>
>NUMA_NO_NODE is the iteration logic, right? It resets the batching node.
>Node check read from the userspace is an input sanitization. Do not put
>those two into the same checks. More clear now?

Yes, I see your point.

Can we think like this:

  On each iteration, we do an input sanitization?

Well, this is a trivial one. If you don't like it, I would remove this.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round

[Michal Hocko]

On Wed 22-01-20 08:36:50, Wei Yang wrote:
[...]
> Well, this is a trivial one. If you don't like it, I would remove this.

Yes, please drop it. Thanks!
-- 
Michal Hocko
SUSE Labs

[PATCH 2/8] mm/migrate.c: not necessary to check start and i

[Wei Yang]

Till here, i must no less than start. And if i equals to start,
store_status() would always return 0.

Remove some unnecessary check to make it easy to read and prepare for
further cleanup.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index ba7cf4fa43a0..c3ef70de5876 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		err = do_move_pages_to_node(mm, &pagelist, current_node);
 		if (err)
 			goto out;
-		if (i > start) {
-			err = store_status(status, start, current_node, i - start);
-			if (err)
-				goto out;
-		}
+		err = store_status(status, start, current_node, i - start);
+		if (err)
+			goto out;
 		current_node = NUMA_NO_NODE;
 	}
 out_flush:
-- 
2.17.1

Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i

[David Rientjes]

On Sun, 19 Jan 2020, Wei Yang wrote:

> diff --git a/mm/migrate.c b/mm/migrate.c
> index ba7cf4fa43a0..c3ef70de5876 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		err = do_move_pages_to_node(mm, &pagelist, current_node);
>  		if (err)
>  			goto out;
> -		if (i > start) {
> -			err = store_status(status, start, current_node, i - start);
> -			if (err)
> -				goto out;
> -		}
> +		err = store_status(status, start, current_node, i - start);
> +		if (err)
> +			goto out;
>  		current_node = NUMA_NO_NODE;
>  	}
>  out_flush:

Not sure this is useful, it relies on the implementation of store_status() 
when i == start and the overhead of the function call should actually be 
slower than the simple conditional to avoid it in that case?

Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i

[Wei Yang]

On Sun, Jan 19, 2020 at 02:14:28PM -0800, David Rientjes wrote:
>On Sun, 19 Jan 2020, Wei Yang wrote:
>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index ba7cf4fa43a0..c3ef70de5876 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  		err = do_move_pages_to_node(mm, &pagelist, current_node);
>>  		if (err)
>>  			goto out;
>> -		if (i > start) {
>> -			err = store_status(status, start, current_node, i - start);
>> -			if (err)
>> -				goto out;
>> -		}
>> +		err = store_status(status, start, current_node, i - start);
>> +		if (err)
>> +			goto out;
>>  		current_node = NUMA_NO_NODE;
>>  	}
>>  out_flush:
>
>Not sure this is useful, it relies on the implementation of store_status() 
>when i == start and the overhead of the function call should actually be 
>slower than the simple conditional to avoid it in that case?

Not sure about this.

While this patch is a transient state for the following cleanup. The purpose
of this is to make the consolidation a little easy to review.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i

[Michal Hocko]

On Sun 19-01-20 11:06:30, Wei Yang wrote:
> Till here, i must no less than start. And if i equals to start,
> store_status() would always return 0.
> 
> Remove some unnecessary check to make it easy to read and prepare for
> further cleanup.

You are right. This is likely a left over from the development.
i >= start because the former is the actual iterator and start is the
first index with the cached node.

Dropping the check improves readability because one might indeed wonder
why this is the only place to do the check and the overal iteration is
complex enough to add more questions on top.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/migrate.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index ba7cf4fa43a0..c3ef70de5876 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		err = do_move_pages_to_node(mm, &pagelist, current_node);
>  		if (err)
>  			goto out;
> -		if (i > start) {
> -			err = store_status(status, start, current_node, i - start);
> -			if (err)
> -				goto out;
> -		}
> +		err = store_status(status, start, current_node, i - start);
> +		if (err)
> +			goto out;
>  		current_node = NUMA_NO_NODE;
>  	}
>  out_flush:
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

[PATCH 3/8] mm/migrate.c: reform the last call on do_move_pages_to_node()

[Wei Yang]

No functional change, just reform it to make it as the same shape as
other calls on do_move_pages_to_node().

This is a preparation for further cleanup.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index c3ef70de5876..4a63fb8fbb6d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 
 	/* Make sure we do not overwrite the existing error */
 	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
-	if (!err1)
-		err1 = store_status(status, start, current_node, i - start);
+	if (err1) {
+		if (err >= 0)
+			err = err1;
+		goto out;
+	}
+	err1 = store_status(status, start, current_node, i - start);
 	if (err >= 0)
 		err = err1;
 out:
-- 
2.17.1

Re: [PATCH 3/8] mm/migrate.c: reform the last call on do_move_pages_to_node()

[Michal Hocko]

On Sun 19-01-20 11:06:31, Wei Yang wrote:
> No functional change, just reform it to make it as the same shape as
> other calls on do_move_pages_to_node().
> 
> This is a preparation for further cleanup.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/migrate.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index c3ef70de5876..4a63fb8fbb6d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  
>  	/* Make sure we do not overwrite the existing error */
>  	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
> -	if (!err1)
> -		err1 = store_status(status, start, current_node, i - start);
> +	if (err1) {
> +		if (err >= 0)
> +			err = err1;
> +		goto out;
> +	}
> +	err1 = store_status(status, start, current_node, i - start);

Please don't. This just makes the code harder to follow. The current err
and err1 is already quite ugly so do not make it more so.

>  	if (err >= 0)
>  		err = err1;
>  out:
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 3/8] mm/migrate.c: reform the last call on do_move_pages_to_node()

[Wei Yang]

On Mon, Jan 20, 2020 at 10:46:08AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:31, Wei Yang wrote:
>> No functional change, just reform it to make it as the same shape as
>> other calls on do_move_pages_to_node().
>> 
>> This is a preparation for further cleanup.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  mm/migrate.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index c3ef70de5876..4a63fb8fbb6d 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>  
>>  	/* Make sure we do not overwrite the existing error */
>>  	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
>> -	if (!err1)
>> -		err1 = store_status(status, start, current_node, i - start);
>> +	if (err1) {
>> +		if (err >= 0)
>> +			err = err1;
>> +		goto out;
>> +	}
>> +	err1 = store_status(status, start, current_node, i - start);
>
>Please don't. This just makes the code harder to follow. The current err
>and err1 is already quite ugly so do not make it more so.
>

Yes, I struggled a little on doing this change. Sounds we can merge this one
with the following consolidation.

>>  	if (err >= 0)
>>  		err = err1;
>>  out:
>> -- 
>> 2.17.1
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[PATCH 4/8] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[Wei Yang]

Usually do_move_pages_to_node() and store_status() is a pair. There are
three places call this pair of functions with almost the same form.

This patch just wrap it to make it friendly to audience and also
consolidate the move and store action into one place. Also mentioned by
Yang Shi, the handling of do_move_pages_to_node()'s return value is not
proper. Now we can fix it in one place.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index 4a63fb8fbb6d..dec147d3a4dd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	return err;
 }
 
+static int move_pages_and_store_status(struct mm_struct *mm, int node,
+		struct list_head *pagelist, int __user *status,
+		int start, int nr)
+{
+	int err;
+
+	err = do_move_pages_to_node(mm, pagelist, node);
+	if (err)
+		return err;
+	err = store_status(status, start, node, nr);
+	return err;
+}
+
 /*
  * Migrate an array of page address onto an array of nodes and fill
  * the corresponding array of status.
@@ -1629,10 +1642,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			current_node = node;
 			start = i;
 		} else if (node != current_node) {
-			err = do_move_pages_to_node(mm, &pagelist, current_node);
-			if (err)
-				goto out;
-			err = store_status(status, start, current_node, i - start);
+			err = move_pages_and_store_status(mm, current_node,
+					&pagelist, status, start, i - start);
 			if (err)
 				goto out;
 			start = i;
@@ -1661,10 +1672,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		if (err)
 			goto out_flush;
 
-		err = do_move_pages_to_node(mm, &pagelist, current_node);
-		if (err)
-			goto out;
-		err = store_status(status, start, current_node, i - start);
+		err = move_pages_and_store_status(mm, current_node, &pagelist,
+				status, start, i - start);
 		if (err)
 			goto out;
 		current_node = NUMA_NO_NODE;
@@ -1674,13 +1683,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		return err;
 
 	/* Make sure we do not overwrite the existing error */
-	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
-	if (err1) {
-		if (err >= 0)
-			err = err1;
-		goto out;
-	}
-	err1 = store_status(status, start, current_node, i - start);
+	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
+				status, start, i - start);
 	if (err >= 0)
 		err = err1;
 out:
-- 
2.17.1

Re: [PATCH 4/8] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[Michal Hocko]

On Sun 19-01-20 11:06:32, Wei Yang wrote:
> Usually do_move_pages_to_node() and store_status() is a pair. There are
> three places call this pair of functions with almost the same form.
> 
> This patch just wrap it to make it friendly to audience and also
> consolidate the move and store action into one place. Also mentioned by
> Yang Shi, the handling of do_move_pages_to_node()'s return value is not
> proper. Now we can fix it in one place.

The helper helps here indeed. Thanks this makes the code easier to read.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/migrate.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4a63fb8fbb6d..dec147d3a4dd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>  	return err;
>  }
>  
> +static int move_pages_and_store_status(struct mm_struct *mm, int node,
> +		struct list_head *pagelist, int __user *status,
> +		int start, int nr)
> +{
> +	int err;
> +
> +	err = do_move_pages_to_node(mm, pagelist, node);
> +	if (err)
> +		return err;
> +	err = store_status(status, start, node, nr);
> +	return err;
> +}
> +
>  /*
>   * Migrate an array of page address onto an array of nodes and fill
>   * the corresponding array of status.
> @@ -1629,10 +1642,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  			current_node = node;
>  			start = i;
>  		} else if (node != current_node) {
> -			err = do_move_pages_to_node(mm, &pagelist, current_node);
> -			if (err)
> -				goto out;
> -			err = store_status(status, start, current_node, i - start);
> +			err = move_pages_and_store_status(mm, current_node,
> +					&pagelist, status, start, i - start);
>  			if (err)
>  				goto out;
>  			start = i;
> @@ -1661,10 +1672,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		if (err)
>  			goto out_flush;
>  
> -		err = do_move_pages_to_node(mm, &pagelist, current_node);
> -		if (err)
> -			goto out;
> -		err = store_status(status, start, current_node, i - start);
> +		err = move_pages_and_store_status(mm, current_node, &pagelist,
> +				status, start, i - start);
>  		if (err)
>  			goto out;
>  		current_node = NUMA_NO_NODE;
> @@ -1674,13 +1683,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		return err;
>  
>  	/* Make sure we do not overwrite the existing error */
> -	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
> -	if (err1) {
> -		if (err >= 0)
> -			err = err1;
> -		goto out;
> -	}
> -	err1 = store_status(status, start, current_node, i - start);
> +	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
> +				status, start, i - start);
>  	if (err >= 0)
>  		err = err1;
>  out:
> -- 
> 2.17.1
> 

-- 
Michal Hocko
SUSE Labs

[PATCH 5/8] mm/migrate.c: check pagelist in move_pages_and_store_status()

[Wei Yang]

When pagelist is empty, it is not necessary to do the move and store.
Also it consolidate the empty list check in one place.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index dec147d3a4dd..46a5697b7fc6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm,
 {
 	int err;
 
-	if (list_empty(pagelist))
-		return 0;
-
 	err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
 			MIGRATE_SYNC, MR_SYSCALL);
 	if (err)
@@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node,
 {
 	int err;
 
+	if (list_empty(pagelist))
+		return 0;
+
 	err = do_move_pages_to_node(mm, pagelist, node);
 	if (err)
 		return err;
@@ -1679,9 +1679,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		current_node = NUMA_NO_NODE;
 	}
 out_flush:
-	if (list_empty(&pagelist))
-		return err;
-
 	/* Make sure we do not overwrite the existing error */
 	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
 				status, start, i - start);
-- 
2.17.1

Re: [PATCH 5/8] mm/migrate.c: check pagelist in move_pages_and_store_status()

[Michal Hocko]

On Sun 19-01-20 11:06:33, Wei Yang wrote:
> When pagelist is empty, it is not necessary to do the move and store.
> Also it consolidate the empty list check in one place.

OK looks good to me.
 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/migrate.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dec147d3a4dd..46a5697b7fc6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm,
>  {
>  	int err;
>  
> -	if (list_empty(pagelist))
> -		return 0;
> -
>  	err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
>  			MIGRATE_SYNC, MR_SYSCALL);
>  	if (err)
> @@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node,
>  {
>  	int err;
>  
> +	if (list_empty(pagelist))
> +		return 0;
> +
>  	err = do_move_pages_to_node(mm, pagelist, node);
>  	if (err)
>  		return err;
> @@ -1679,9 +1679,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		current_node = NUMA_NO_NODE;
>  	}
>  out_flush:
> -	if (list_empty(&pagelist))
> -		return err;
> -
>  	/* Make sure we do not overwrite the existing error */
>  	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
>  				status, start, i - start);
> -- 
> 2.17.1
> 

-- 
Michal Hocko
SUSE Labs

[PATCH 6/8] mm/migrate.c: handle same node and add failure in the same way

[Wei Yang]

When page is not queued for migration, there are two possible cases:

  * page already on the target node
  * failed to add to migration queue

Current code handle them differently, this leads to a behavior
inconsistency.

Usually for each page's status, we just do store for once. While for the
page already on the target node, we might store the node information for
twice:

  * once when we found the page is on the target node
  * second when moving the pages to target node successfully after above
    action

The reason is even we don't add the page to pagelist, but store_status()
does store in a range which still contains the page.

This patch handles these two cases in the same way to reduce this
inconsistency and also make the code a little easier to read.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index 46a5697b7fc6..aee5aeb082c4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1657,18 +1657,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		err = add_page_for_migration(mm, addr, current_node,
 				&pagelist, flags & MPOL_MF_MOVE_ALL);
 
-		if (!err) {
-			/* The page is already on the target node */
-			err = store_status(status, i, current_node, 1);
-			if (err)
-				goto out_flush;
-			continue;
-		} else if (err > 0) {
+		if (err > 0) {
 			/* The page is successfully queued for migration */
 			continue;
 		}
 
-		err = store_status(status, i, err, 1);
+		/*
+		 * Two possible cases for err here:
+		 * == 0: page is already on the target node, then store
+		 *       current_node to status
+		 * <  0: failed to add page to list, then store err to status
+		 */
+		err = store_status(status, i, err ? : current_node, 1);
 		if (err)
 			goto out_flush;
 
-- 
2.17.1

Re: [PATCH 6/8] mm/migrate.c: handle same node and add failure in the same way

[Michal Hocko]

On Sun 19-01-20 11:06:34, Wei Yang wrote:
> When page is not queued for migration, there are two possible cases:
> 
>   * page already on the target node
>   * failed to add to migration queue
> 
> Current code handle them differently, this leads to a behavior
> inconsistency.
> 
> Usually for each page's status, we just do store for once. While for the
> page already on the target node, we might store the node information for
> twice:
> 
>   * once when we found the page is on the target node
>   * second when moving the pages to target node successfully after above
>     action
> 
> The reason is even we don't add the page to pagelist, but store_status()
> does store in a range which still contains the page.
> 
> This patch handles these two cases in the same way to reduce this
> inconsistency and also make the code a little easier to read.

Yeah, the improvement is really marginal. I do not feel strongly one way
or another.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/migrate.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 46a5697b7fc6..aee5aeb082c4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1657,18 +1657,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  		err = add_page_for_migration(mm, addr, current_node,
>  				&pagelist, flags & MPOL_MF_MOVE_ALL);
>  
> -		if (!err) {
> -			/* The page is already on the target node */
> -			err = store_status(status, i, current_node, 1);
> -			if (err)
> -				goto out_flush;
> -			continue;
> -		} else if (err > 0) {
> +		if (err > 0) {
>  			/* The page is successfully queued for migration */
>  			continue;
>  		}
>  
> -		err = store_status(status, i, err, 1);
> +		/*
> +		 * Two possible cases for err here:
> +		 * == 0: page is already on the target node, then store
> +		 *       current_node to status
> +		 * <  0: failed to add page to list, then store err to status
> +		 */
> +		err = store_status(status, i, err ? : current_node, 1);
>  		if (err)
>  			goto out_flush;
>  
> -- 
> 2.17.1
> 

-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

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(-)

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

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

[Michal Hocko]

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
> 

-- 
Michal Hocko
SUSE Labs

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@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

-- 
Wei Yang
Help you, Help me

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

[Michal Hocko]

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?
-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me

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

[Michal Hocko]

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.
-- 
Michal Hocko
SUSE Labs

[PATCH 8/8] mm/migrate.c: use break instead of goto out_flush

[Wei Yang]

Label out_flush is just outside the loop, so break the loop is enough.

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

diff --git a/mm/migrate.c b/mm/migrate.c
index 2a857fec65b6..59bfae11b9d6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1621,22 +1621,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 
 		err = -EFAULT;
 		if (get_user(p, pages + i))
-			goto out_flush;
+			break;
 		if (get_user(node, nodes + i))
-			goto out_flush;
+			break;
 		addr = (unsigned long)untagged_addr(p);
 
 		/* Check node if it is not checked. */
 		if (current_node == NUMA_NO_NODE || node != current_node) {
 			err = -ENODEV;
 			if (node < 0 || node >= MAX_NUMNODES)
-				goto out_flush;
+				break;
 			if (!node_state(node, N_MEMORY))
-				goto out_flush;
+				break;
 
 			err = -EACCES;
 			if (!node_isset(node, task_nodes))
-				goto out_flush;
+				break;
 		}
 
 		if (current_node == NUMA_NO_NODE) {
@@ -1676,9 +1676,9 @@ 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;
+			break;
 	}
-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 - need_move);
-- 
2.17.1

Re: [PATCH 8/8] mm/migrate.c: use break instead of goto out_flush

[Michal Hocko]

On Sun 19-01-20 11:06:36, Wei Yang wrote:
> Label out_flush is just outside the loop, so break the loop is enough.

I find the explicit label easier to follow than breaking out of the
loop. Especially when there is another break out of the loop with a
different label.

While the patch is correct I find the resulting code worse readable.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  mm/migrate.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 2a857fec65b6..59bfae11b9d6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1621,22 +1621,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  
>  		err = -EFAULT;
>  		if (get_user(p, pages + i))
> -			goto out_flush;
> +			break;
>  		if (get_user(node, nodes + i))
> -			goto out_flush;
> +			break;
>  		addr = (unsigned long)untagged_addr(p);
>  
>  		/* Check node if it is not checked. */
>  		if (current_node == NUMA_NO_NODE || node != current_node) {
>  			err = -ENODEV;
>  			if (node < 0 || node >= MAX_NUMNODES)
> -				goto out_flush;
> +				break;
>  			if (!node_state(node, N_MEMORY))
> -				goto out_flush;
> +				break;
>  
>  			err = -EACCES;
>  			if (!node_isset(node, task_nodes))
> -				goto out_flush;
> +				break;
>  		}
>  
>  		if (current_node == NUMA_NO_NODE) {
> @@ -1676,9 +1676,9 @@ 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;
> +			break;
>  	}
> -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 - need_move);
> -- 
> 2.17.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 8/8] mm/migrate.c: use break instead of goto out_flush

[Wei Yang]

On Mon, Jan 20, 2020 at 11:03:28AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:36, Wei Yang wrote:
>> Label out_flush is just outside the loop, so break the loop is enough.
>
>I find the explicit label easier to follow than breaking out of the
>loop. Especially when there is another break out of the loop with a
>different label.
>
>While the patch is correct I find the resulting code worse readable.
>

ok, as you wish.


-- 
Wei Yang
Help you, Help me