[Patch v2 0/4] cleanup on do_pages_move()

[Patch v2 0/4] 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.

After this, we can do a better fix.

v2:
  * remove some unnecessary cleanup

Wei Yang (4):
  mm/migrate.c: not necessary to check start and i
  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 | 57 +++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

-- 
2.17.1

[Patch v2 1/4] 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>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/migrate.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 430fdccc733e..4c2a21856717 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1661,11 +1661,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

[Patch v2 2/4] 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>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 4c2a21856717..a4d3bd6475e1 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.
@@ -1626,10 +1639,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;
@@ -1658,10 +1669,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;
@@ -1671,9 +1680,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)
-		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

[Patch v2 3/4] 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>
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 a4d3bd6475e1..80d2bba57265 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;
@@ -1676,9 +1676,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

[Patch v2 4/4] 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>
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 80d2bba57265..591f2e5caed6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1654,18 +1654,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 v2 1/4] mm/migrate.c: not necessary to check start and i

[David Hildenbrand]

On 22.01.20 02:16, Wei Yang wrote:
> Till here, i must no less than start. And if i equals to start,
> store_status() would always return 0.

I'd suggest

"
mm/migrate.c: no need to check for i > start in do_pages_move()

At this point, we always have i >= start. If i == start, store_status()
will return 0. So we can drop the check for i > start.
"

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> 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>
> Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/migrate.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 430fdccc733e..4c2a21856717 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1661,11 +1661,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:
> 


-- 
Thanks,

David / dhildenb

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[David Hildenbrand]

On 22.01.20 02:16, 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.

I'd suggest

"
Usually, do_move_pages_to_node() and store_status() are used in
combination. We have three similar call sites.

Let's provide a wrapper for both function calls -
move_pages_and_store_status - to make the calling code easier to
maintain and fix (as noted by Yang Shi, the return value handling of
do_move_pages_to_node() has a flaw).
"

> 
> 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>
> Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/migrate.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4c2a21856717..a4d3bd6475e1 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)

nit: indentation

> +{
> +	int err;
> +
> +	err = do_move_pages_to_node(mm, pagelist, node);
> +	if (err)
> +		return err;
> +	err = store_status(status, start, node, nr);
> +	return err;

return store_status(status, start, node, nr);

directly



Apart from that (and some more indentation nits)

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

[David Hildenbrand]

On 22.01.20 02:16, 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.
> 
> 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 a4d3bd6475e1..80d2bba57265 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;
> @@ -1676,9 +1676,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;


Am I wrong or are you discarding an error here? (err could be !0)


-- 
Thanks,

David / dhildenb

Re: [Patch v2 1/4] mm/migrate.c: not necessary to check start and i

[Wei Yang]

On Tue, Jan 28, 2020 at 11:04:23AM +0100, David Hildenbrand wrote:
>On 22.01.20 02:16, Wei Yang wrote:
>> Till here, i must no less than start. And if i equals to start,
>> store_status() would always return 0.
>
>I'd suggest
>
>"
>mm/migrate.c: no need to check for i > start in do_pages_move()
>
>At this point, we always have i >= start. If i == start, store_status()
>will return 0. So we can drop the check for i > start.
>"
>
>Reviewed-by: David Hildenbrand <david@redhat.com>
>

Thanks, I took it.

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[Wei Yang]

On Tue, Jan 28, 2020 at 11:14:55AM +0100, David Hildenbrand wrote:
>On 22.01.20 02:16, 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.
>
>I'd suggest
>
>"
>Usually, do_move_pages_to_node() and store_status() are used in
>combination. We have three similar call sites.
>
>Let's provide a wrapper for both function calls -
>move_pages_and_store_status - to make the calling code easier to
>maintain and fix (as noted by Yang Shi, the return value handling of
>do_move_pages_to_node() has a flaw).
>"

Looks good.

>
>> 
>> 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>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> ---
>>  mm/migrate.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>> 
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 4c2a21856717..a4d3bd6475e1 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)
>
>nit: indentation
>

You mean indent like this?

static int move_pages_and_store_status(struct mm_struct *mm, int node,
				       struct list_head *pagelist,
				       int __user *status,

This would be along list and I am afraid this is not the only valid code
style?

>> +{
>> +	int err;
>> +
>> +	err = do_move_pages_to_node(mm, pagelist, node);
>> +	if (err)
>> +		return err;
>> +	err = store_status(status, start, node, nr);
>> +	return err;
>
>return store_status(status, start, node, nr);
>
>directly
>

ok

>
>
>Apart from that (and some more indentation nits)
>
>Reviewed-by: David Hildenbrand <david@redhat.com>
>
>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

[Wei Yang]

On Tue, Jan 28, 2020 at 11:21:13AM +0100, David Hildenbrand wrote:
>On 22.01.20 02:16, 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.
>> 
>> 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 a4d3bd6475e1..80d2bba57265 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;
>> @@ -1676,9 +1676,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;
>
>
>Am I wrong or are you discarding an error here? (err could be !0)
>

This is not obvious in this code snippet. If you look into the source code, it
might help you get the whole picture.

The following comment says:

  Make sure we do not overwrite the existing error

So we didn't eat error here.

>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[David Hildenbrand]

On 29.01.20 01:38, Wei Yang wrote:
> On Tue, Jan 28, 2020 at 11:14:55AM +0100, David Hildenbrand wrote:
>> On 22.01.20 02:16, 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.
>>
>> I'd suggest
>>
>> "
>> Usually, do_move_pages_to_node() and store_status() are used in
>> combination. We have three similar call sites.
>>
>> Let's provide a wrapper for both function calls -
>> move_pages_and_store_status - to make the calling code easier to
>> maintain and fix (as noted by Yang Shi, the return value handling of
>> do_move_pages_to_node() has a flaw).
>> "
> 
> Looks good.
> 
>>
>>>
>>> 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>
>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>> ---
>>>  mm/migrate.c | 30 +++++++++++++++++++-----------
>>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 4c2a21856717..a4d3bd6475e1 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)
>>
>> nit: indentation
>>
> 
> You mean indent like this?
> 
> static int move_pages_and_store_status(struct mm_struct *mm, int node,
> 				       struct list_head *pagelist,
> 				       int __user *status,
> 
> This would be along list and I am afraid this is not the only valid code
> style?

Yes, that's what I meant. Documentation/process/coding-style.rst doesn't
mention any specific way, but this is the most commonly used one.

Indentation in this file mostly sticks to something like this as well,
but yeah, it's often a mess and not consistent.

That's why I note it whenever I see it, to make it eventually more
consistent (and only make it a nit) :)

-- 
Thanks,

David / dhildenb

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

[David Hildenbrand]

On 29.01.20 01:46, Wei Yang wrote:
> On Tue, Jan 28, 2020 at 11:21:13AM +0100, David Hildenbrand wrote:
>> On 22.01.20 02:16, 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.
>>>
>>> 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 a4d3bd6475e1..80d2bba57265 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;
>>> @@ -1676,9 +1676,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;
>>
>>
>> Am I wrong or are you discarding an error here? (err could be !0)
>>
> 
> This is not obvious in this code snippet. If you look into the source code, it
> might help you get the whole picture.
> 
> The following comment says:
> 
>   Make sure we do not overwrite the existing error
> 
> So we didn't eat error here.

My fault, I misread the "if (err >= 0)" (and thought it was a "if (err1
>= 0)")

Makes perfect sense, thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [Patch v2 4/4] mm/migrate.c: handle same node and add failure in the same way

[David Hildenbrand]

On 22.01.20 02:16, 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.
> 

I'd rephrase to

"mm/migrate.c: unify "not queued for migration" handling in do_pages_move()

It can currently happen that we store the status of a page twice:
* Once we detect that it is already on the target node
* Once we moved a bunch of pages, and a page that's already on the
  target node is contained in the current interval.

Let's simplify the code and always call do_move_pages_to_node() in
case we did not queue a page for migration. Note that pages that are
already on the target node are not added to the pagelist and are,
therefore, ignored by do_move_pages_to_node() - there is no functional
change.

The status of such a page is now only stored once.
"

> 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 80d2bba57265..591f2e5caed6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1654,18 +1654,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
> +		 */

I'd shorten that to

/*
 * If the page is already on the target node (!err), store the node,
 * otherwise, store the err.
*/

> +		err = store_status(status, i, err ? : current_node, 1);
>  		if (err)
>  			goto out_flush;
>  
> 

Thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

[Wei Yang]

On Wed, Jan 29, 2020 at 10:28:53AM +0100, David Hildenbrand wrote:
>On 29.01.20 01:38, Wei Yang wrote:
>> On Tue, Jan 28, 2020 at 11:14:55AM +0100, David Hildenbrand wrote:
>>> On 22.01.20 02:16, 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.
>>>
>>> I'd suggest
>>>
>>> "
>>> Usually, do_move_pages_to_node() and store_status() are used in
>>> combination. We have three similar call sites.
>>>
>>> Let's provide a wrapper for both function calls -
>>> move_pages_and_store_status - to make the calling code easier to
>>> maintain and fix (as noted by Yang Shi, the return value handling of
>>> do_move_pages_to_node() has a flaw).
>>> "
>> 
>> Looks good.
>> 
>>>
>>>>
>>>> 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>
>>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>> ---
>>>>  mm/migrate.c | 30 +++++++++++++++++++-----------
>>>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index 4c2a21856717..a4d3bd6475e1 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)
>>>
>>> nit: indentation
>>>
>> 
>> You mean indent like this?
>> 
>> static int move_pages_and_store_status(struct mm_struct *mm, int node,
>> 				       struct list_head *pagelist,
>> 				       int __user *status,
>> 
>> This would be along list and I am afraid this is not the only valid code
>> style?
>
>Yes, that's what I meant. Documentation/process/coding-style.rst doesn't
>mention any specific way, but this is the most commonly used one.
>
>Indentation in this file mostly sticks to something like this as well,
>but yeah, it's often a mess and not consistent.
>
>That's why I note it whenever I see it, to make it eventually more
>consistent (and only make it a nit) :)
>

You are right. I would leave as it now, if someone else still have the same
suggestion to fix it.

Thanks :-)

>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 4/4] mm/migrate.c: handle same node and add failure in the same way

[Wei Yang]

On Wed, Jan 29, 2020 at 11:13:23AM +0100, David Hildenbrand wrote:
>On 22.01.20 02:16, 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.
>> 
>
>I'd rephrase to
>
>"mm/migrate.c: unify "not queued for migration" handling in do_pages_move()
>
>It can currently happen that we store the status of a page twice:
>* Once we detect that it is already on the target node
>* Once we moved a bunch of pages, and a page that's already on the
>  target node is contained in the current interval.
>
>Let's simplify the code and always call do_move_pages_to_node() in
>case we did not queue a page for migration. Note that pages that are
>already on the target node are not added to the pagelist and are,
>therefore, ignored by do_move_pages_to_node() - there is no functional
>change.
>
>The status of such a page is now only stored once.
>"
>
>> 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 80d2bba57265..591f2e5caed6 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1654,18 +1654,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
>> +		 */
>
>I'd shorten that to
>
>/*
> * If the page is already on the target node (!err), store the node,
> * otherwise, store the err.
>*/
>
>> +		err = store_status(status, i, err ? : current_node, 1);
>>  		if (err)
>>  			goto out_flush;
>>  
>> 
>
>Thanks!
>
>Reviewed-by: David Hildenbrand <david@redhat.com>
>

Yep, thanks.

I would take this :-)

>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me