diff mbox series

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

Message ID 20200122011647.13636-3-richardw.yang@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series cleanup on do_pages_move() | expand

Commit Message

Wei Yang Jan. 22, 2020, 1:16 a.m. UTC 
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(-)

Comments

David Hildenbrand Jan. 28, 2020, 10:14 a.m. UTC | #1 
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>
Wei Yang Jan. 29, 2020, 12:38 a.m. UTC | #2 
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
David Hildenbrand Jan. 29, 2020, 9:28 a.m. UTC | #3 
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) :)
Wei Yang Jan. 29, 2020, 10 p.m. UTC | #4 
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
diff mbox series

Patch

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: