[v2 PATCH] mm: migrate: handle freed page at the first place

[v2 PATCH] mm: migrate: handle freed page at the first place

[Yang Shi]

When doing migration if the freed page is met, we just return without
migrating it since it is pointless to migrate a freed page.  But, the
current code allocates target page unconditionally before handling freed
page, if the page is freed, the newly allocated will be just freed.  It
doesn't make too much sense and is just a waste of time although
migrating freed page is rare.

So, handle freed page at the before that to avoid unnecessary page
allocation and free.

Cc: Michal Hocko <mhocko@suse.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
v2: * Keep thp migration support check before handling freed page per Michal Hocko
    * Fixed the build warning reported by 0-day

 mm/migrate.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 4fe45d1..a8f87cb 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 				   enum migrate_reason reason)
 {
 	int rc = MIGRATEPAGE_SUCCESS;
-	struct page *newpage;
+	struct page *newpage = NULL;
 
 	if (!thp_migration_supported() && PageTransHuge(page))
 		return -ENOMEM;
 
-	newpage = get_new_page(page, private);
-	if (!newpage)
-		return -ENOMEM;
-
 	if (page_count(page) == 1) {
 		/* page was freed from under us. So we are done. */
 		ClearPageActive(page);
@@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 				__ClearPageIsolated(page);
 			unlock_page(page);
 		}
-		if (put_new_page)
-			put_new_page(newpage, private);
-		else
-			put_page(newpage);
 		goto out;
 	}
 
+	newpage = get_new_page(page, private);
+	if (!newpage)
+		return -ENOMEM;
+
 	rc = __unmap_and_move(page, newpage, force, mode);
 	if (rc == MIGRATEPAGE_SUCCESS)
 		set_page_owner_migrate_reason(newpage, reason);
-- 
1.8.3.1

Re: [v2 PATCH] mm: migrate: handle freed page at the first place

[Michal Hocko]

On Fri 15-11-19 02:24:29, Yang Shi wrote:
> When doing migration if the freed page is met, we just return without
> migrating it since it is pointless to migrate a freed page.  But, the
> current code allocates target page unconditionally before handling freed
> page, if the page is freed, the newly allocated will be just freed.  It
> doesn't make too much sense and is just a waste of time although
> migrating freed page is rare.
> 
> So, handle freed page at the before that to avoid unnecessary page
> allocation and free.
> 
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>

I would be really surprised if this led to any runtime visible effect
but I do agree that one less put_page path looks slightly better. For
that reason
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
> v2: * Keep thp migration support check before handling freed page per Michal Hocko
>     * Fixed the build warning reported by 0-day
> 
>  mm/migrate.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4fe45d1..a8f87cb 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  				   enum migrate_reason reason)
>  {
>  	int rc = MIGRATEPAGE_SUCCESS;
> -	struct page *newpage;
> +	struct page *newpage = NULL;
>  
>  	if (!thp_migration_supported() && PageTransHuge(page))
>  		return -ENOMEM;
>  
> -	newpage = get_new_page(page, private);
> -	if (!newpage)
> -		return -ENOMEM;
> -
>  	if (page_count(page) == 1) {
>  		/* page was freed from under us. So we are done. */
>  		ClearPageActive(page);
> @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  				__ClearPageIsolated(page);
>  			unlock_page(page);
>  		}
> -		if (put_new_page)
> -			put_new_page(newpage, private);
> -		else
> -			put_page(newpage);
>  		goto out;
>  	}
>  
> +	newpage = get_new_page(page, private);
> +	if (!newpage)
> +		return -ENOMEM;
> +
>  	rc = __unmap_and_move(page, newpage, force, mode);
>  	if (rc == MIGRATEPAGE_SUCCESS)
>  		set_page_owner_migrate_reason(newpage, reason);
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [v2 PATCH] mm: migrate: handle freed page at the first place

[Yang Shi]

On 11/14/19 10:56 AM, Michal Hocko wrote:
> On Fri 15-11-19 02:24:29, Yang Shi wrote:
>> When doing migration if the freed page is met, we just return without
>> migrating it since it is pointless to migrate a freed page.  But, the
>> current code allocates target page unconditionally before handling freed
>> page, if the page is freed, the newly allocated will be just freed.  It
>> doesn't make too much sense and is just a waste of time although
>> migrating freed page is rare.
>>
>> So, handle freed page at the before that to avoid unnecessary page
>> allocation and free.
>>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> I would be really surprised if this led to any runtime visible effect
> but I do agree that one less put_page path looks slightly better. For
> that reason
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

>
>> ---
>> v2: * Keep thp migration support check before handling freed page per Michal Hocko
>>      * Fixed the build warning reported by 0-day
>>
>>   mm/migrate.c | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 4fe45d1..a8f87cb 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>   				   enum migrate_reason reason)
>>   {
>>   	int rc = MIGRATEPAGE_SUCCESS;
>> -	struct page *newpage;
>> +	struct page *newpage = NULL;
>>   
>>   	if (!thp_migration_supported() && PageTransHuge(page))
>>   		return -ENOMEM;
>>   
>> -	newpage = get_new_page(page, private);
>> -	if (!newpage)
>> -		return -ENOMEM;
>> -
>>   	if (page_count(page) == 1) {
>>   		/* page was freed from under us. So we are done. */
>>   		ClearPageActive(page);
>> @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>   				__ClearPageIsolated(page);
>>   			unlock_page(page);
>>   		}
>> -		if (put_new_page)
>> -			put_new_page(newpage, private);
>> -		else
>> -			put_page(newpage);
>>   		goto out;
>>   	}
>>   
>> +	newpage = get_new_page(page, private);
>> +	if (!newpage)
>> +		return -ENOMEM;
>> +
>>   	rc = __unmap_and_move(page, newpage, force, mode);
>>   	if (rc == MIGRATEPAGE_SUCCESS)
>>   		set_page_owner_migrate_reason(newpage, reason);
>> -- 
>> 1.8.3.1
>>

Re: [v2 PATCH] mm: migrate: handle freed page at the first place

[lixinhai.lxh]

On 2019-11-15 at 04:58 Yang Shi wrote:
>
>
>On 11/14/19 10:56 AM, Michal Hocko wrote:
>> On Fri 15-11-19 02:24:29, Yang Shi wrote:
>>> When doing migration if the freed page is met, we just return without
>>> migrating it since it is pointless to migrate a freed page.  But, the
>>> current code allocates target page unconditionally before handling freed
>>> page, if the page is freed, the newly allocated will be just freed.  It
>>> doesn't make too much sense and is just a waste of time although
>>> migrating freed page is rare.
>>>
>>> So, handle freed page at the before that to avoid unnecessary page
>>> allocation and free.
>>>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Mel Gorman <mgorman@techsingularity.net>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> I would be really surprised if this led to any runtime visible effect
>> but I do agree that one less put_page path looks slightly better. For
>> that reason
>> Acked-by: Michal Hocko <mhocko@suse.com>
>
>Thanks!
> 

I think the original design wants to complete all preparation work before
real migration starts with lock on both 'page' and 'newpage', and any race
condition is caught right before real work start. (with this patch,
page_count(page)==1 still possible after allocate 'newpage', but we don't
check it. Actually, freeing 'page' would be evenly distributed during the whole
procedure of the preparation phase.)

The __unmap_and_move() is called right after page_count() check, and 
where locking 'page' and 'newpage' is done immediately.

- Xinhai


>>
>>> ---
>>> v2: * Keep thp migration support check before handling freed page per Michal Hocko
>>>      * Fixed the build warning reported by 0-day
>>>
>>>   mm/migrate.c | 14 +++++---------
>>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 4fe45d1..a8f87cb 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>>      enum migrate_reason reason)
>>>   {
>>>   int rc = MIGRATEPAGE_SUCCESS;
>>> -	struct page *newpage;
>>> +	struct page *newpage = NULL;
>>>  
>>>   if (!thp_migration_supported() && PageTransHuge(page))
>>>   return -ENOMEM;
>>>  
>>> -	newpage = get_new_page(page, private);
>>> -	if (!newpage)
>>> -	return -ENOMEM;
>>> -
>>>   if (page_count(page) == 1) {
>>>   /* page was freed from under us. So we are done. */
>>>   ClearPageActive(page);
>>> @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>>   __ClearPageIsolated(page);
>>>   unlock_page(page);
>>>   }
>>> -	if (put_new_page)
>>> -	put_new_page(newpage, private);
>>> -	else
>>> -	put_page(newpage);
>>>   goto out;
>>>   }
>>>  
>>> +	newpage = get_new_page(page, private);
>>> +	if (!newpage)
>>> +	return -ENOMEM;
>>> +
>>>   rc = __unmap_and_move(page, newpage, force, mode);
>>>   if (rc == MIGRATEPAGE_SUCCESS)
>>>   set_page_owner_migrate_reason(newpage, reason);
>>> --
>>> 1.8.3.1
>>>
>
>