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

Message ID 20200119030636.11899-2-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
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(-)

Comments

Michal Hocko Jan. 20, 2020, 9:36 a.m. UTC | #1
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
Wei Yang Jan. 20, 2020, 10:25 p.m. UTC | #2
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
Michal Hocko Jan. 21, 2020, 8:42 a.m. UTC | #3
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?
Wei Yang Jan. 22, 2020, 12:36 a.m. UTC | #4
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
Michal Hocko Jan. 22, 2020, 8:16 a.m. UTC | #5
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!

Patch
diff mbox series

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;