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

[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 did two things before handling freed page:

1. Return -ENOMEM if the page is THP and THP migration is not supported.
2. Allocate target page unconditionally.

Both makes not too much sense.  If we handle freed page at the first place
we don't have to worry about allocating/freeing target page and split
THP at all.

For example (worst case) if we are trying to migrate a freed THP without
THP migration supported, the migrate_pages() would just split the THP then
retry to migrate base pages one by one by pointless allocating and freeing
pages, this is just waste of time.

I didn't run into any actual problem with the current code (or I may
just not notice it yet), it was found by visual inspection.

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>
---
 mm/migrate.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 4fe45d1..ef96997 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 	int rc = MIGRATEPAGE_SUCCESS;
 	struct page *newpage;
 
-	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 +1180,16 @@ 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;
 	}
 
+	if (!thp_migration_supported() && PageTransHuge(page))
+		return -ENOMEM;
+
+	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: [PATCH] mm: migrate: handle freed page at the first place

[Andrew Morton]

On Tue, 12 Nov 2019 06:09:25 +0800 Yang Shi <yang.shi@linux.alibaba.com> 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 did two things before handling freed page:
> 
> 1. Return -ENOMEM if the page is THP and THP migration is not supported.
> 2. Allocate target page unconditionally.
> 
> Both makes not too much sense.  If we handle freed page at the first place
> we don't have to worry about allocating/freeing target page and split
> THP at all.
> 
> For example (worst case) if we are trying to migrate a freed THP without
> THP migration supported, the migrate_pages() would just split the THP then
> retry to migrate base pages one by one by pointless allocating and freeing
> pages, this is just waste of time.
> 
> I didn't run into any actual problem with the current code (or I may
> just not notice it yet), it was found by visual inspection.
> 
>
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  	int rc = MIGRATEPAGE_SUCCESS;
>  	struct page *newpage;
>  
> -	if (!thp_migration_supported() && PageTransHuge(page))
> -		return -ENOMEM;
> -
> -	newpage = get_new_page(page, private);
> -	if (!newpage)
> -		return -ENOMEM;
> -
>  	if (page_count(page) == 1) {

Is it possible to have (!thp_migration_supported() &&
PageTransHuge(page) && page_count(page) == 1)?  If so, isn't this new
behviour?

>  		/* page was freed from under us. So we are done. */
>  		ClearPageActive(page);
> @@ -1187,13 +1180,16 @@ 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;
>  	}
>  
> +	if (!thp_migration_supported() && PageTransHuge(page))
> +		return -ENOMEM;
> +
> +	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);

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

[Yang Shi]

On 11/11/19 3:18 PM, Andrew Morton wrote:
> On Tue, 12 Nov 2019 06:09:25 +0800 Yang Shi <yang.shi@linux.alibaba.com> 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 did two things before handling freed page:
>>
>> 1. Return -ENOMEM if the page is THP and THP migration is not supported.
>> 2. Allocate target page unconditionally.
>>
>> Both makes not too much sense.  If we handle freed page at the first place
>> we don't have to worry about allocating/freeing target page and split
>> THP at all.
>>
>> For example (worst case) if we are trying to migrate a freed THP without
>> THP migration supported, the migrate_pages() would just split the THP then
>> retry to migrate base pages one by one by pointless allocating and freeing
>> pages, this is just waste of time.
>>
>> I didn't run into any actual problem with the current code (or I may
>> just not notice it yet), it was found by visual inspection.
>>
>>
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>   	int rc = MIGRATEPAGE_SUCCESS;
>>   	struct page *newpage;
>>   
>> -	if (!thp_migration_supported() && PageTransHuge(page))
>> -		return -ENOMEM;
>> -
>> -	newpage = get_new_page(page, private);
>> -	if (!newpage)
>> -		return -ENOMEM;
>> -
>>   	if (page_count(page) == 1) {
> Is it possible to have (!thp_migration_supported() &&
> PageTransHuge(page) && page_count(page) == 1)?  If so, isn't this new
> behviour?

IMHO it should be possible on some architectures, i.e. aarch64, with 
anonymous THP. I just saw PowerPC and x86_64 have 
CONFIG_ARCH_ENABLE_THP_MIGRATION selected. I'm not quite sure if I miss 
something.

It should be not new behavior since migrate_pages() should just split 
the THP then retry with base pages one by one. Even though it returns 
-EBUSY due to THP split failure in the current code, the behavior sounds 
problematic. We should not return errno for a freed page, right?

>
>>   		/* page was freed from under us. So we are done. */
>>   		ClearPageActive(page);
>> @@ -1187,13 +1180,16 @@ 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;
>>   	}
>>   
>> +	if (!thp_migration_supported() && PageTransHuge(page))
>> +		return -ENOMEM;
>> +
>> +	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);

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

[Michal Hocko]

On Tue 12-11-19 06:09:25, 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 did two things before handling freed page:
> 
> 1. Return -ENOMEM if the page is THP and THP migration is not supported.
> 2. Allocate target page unconditionally.
> 
> Both makes not too much sense.  If we handle freed page at the first place
> we don't have to worry about allocating/freeing target page and split
> THP at all.
> 
> For example (worst case) if we are trying to migrate a freed THP without
> THP migration supported, the migrate_pages() would just split the THP then
> retry to migrate base pages one by one by pointless allocating and freeing
> pages, this is just waste of time.
> 
> I didn't run into any actual problem with the current code (or I may
> just not notice it yet), it was found by visual inspection.

It would be preferable to accompany a change like this with some actual
numbers. A race with page freeing should be a very rare situation. Maybe
it is not under some workloads but that would better be checked and
documented. I also do not like to do page state changes for THP
migration without a support. I cannot really say this is 100% correct
from top of my head and I do not see a sufficient justification to go
and chase all those tiny details because that is time consuming.

> 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>
> ---
>  mm/migrate.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4fe45d1..ef96997 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  	int rc = MIGRATEPAGE_SUCCESS;
>  	struct page *newpage;
>  
> -	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 +1180,16 @@ 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;
>  	}
>  
> +	if (!thp_migration_supported() && PageTransHuge(page))
> +		return -ENOMEM;
> +
> +	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: [PATCH] mm: migrate: handle freed page at the first place

[Michal Hocko]

On Tue 12-11-19 09:04:01, Michal Hocko wrote:
> On Tue 12-11-19 06:09:25, 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 did two things before handling freed page:
> > 
> > 1. Return -ENOMEM if the page is THP and THP migration is not supported.
> > 2. Allocate target page unconditionally.
> > 
> > Both makes not too much sense.  If we handle freed page at the first place
> > we don't have to worry about allocating/freeing target page and split
> > THP at all.
> > 
> > For example (worst case) if we are trying to migrate a freed THP without
> > THP migration supported, the migrate_pages() would just split the THP then
> > retry to migrate base pages one by one by pointless allocating and freeing
> > pages, this is just waste of time.
> > 
> > I didn't run into any actual problem with the current code (or I may
> > just not notice it yet), it was found by visual inspection.
> 
> It would be preferable to accompany a change like this with some actual
> numbers. A race with page freeing should be a very rare situation. Maybe
> it is not under some workloads but that would better be checked and
> documented. I also do not like to do page state changes for THP
> migration without a support. I cannot really say this is 100% correct
> from top of my head and I do not see a sufficient justification to go
> and chase all those tiny details because that is time consuming.

And I forgot to mention one thing. I wouldn't be really opposed to
moving the allocation after the race check because that makes sense even
when the race is rare but moving the thp support check down is far from
clear without a much better justification. 
-- 
Michal Hocko
SUSE Labs

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

[Yang Shi]

On 11/12/19 12:04 AM, Michal Hocko wrote:
> On Tue 12-11-19 06:09:25, 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 did two things before handling freed page:
>>
>> 1. Return -ENOMEM if the page is THP and THP migration is not supported.
>> 2. Allocate target page unconditionally.
>>
>> Both makes not too much sense.  If we handle freed page at the first place
>> we don't have to worry about allocating/freeing target page and split
>> THP at all.
>>
>> For example (worst case) if we are trying to migrate a freed THP without
>> THP migration supported, the migrate_pages() would just split the THP then
>> retry to migrate base pages one by one by pointless allocating and freeing
>> pages, this is just waste of time.
>>
>> I didn't run into any actual problem with the current code (or I may
>> just not notice it yet), it was found by visual inspection.
> It would be preferable to accompany a change like this with some actual
> numbers. A race with page freeing should be a very rare situation. Maybe
> it is not under some workloads but that would better be checked and
> documented. I also do not like to do page state changes for THP
> migration without a support. I cannot really say this is 100% correct

However that THP will not be migrated actually, just will be freed by 
put_page().

> from top of my head and I do not see a sufficient justification to go
> and chase all those tiny details because that is time consuming.
>
>> 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>
>> ---
>>   mm/migrate.c | 18 +++++++-----------
>>   1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 4fe45d1..ef96997 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>>   	int rc = MIGRATEPAGE_SUCCESS;
>>   	struct page *newpage;
>>   
>> -	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 +1180,16 @@ 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;
>>   	}
>>   
>> +	if (!thp_migration_supported() && PageTransHuge(page))
>> +		return -ENOMEM;
>> +
>> +	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: [PATCH] mm: migrate: handle freed page at the first place

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3459 bytes --]

Hi Yang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mmotm/master]
[also build test WARNING on v5.4-rc7 next-20191112]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Yang-Shi/mm-migrate-handle-freed-page-at-the-first-place/20191113-044923
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: x86_64-rhel-7.6-kasan (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:5:0,
                    from arch/x86/include/asm/bug.h:83,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from include/linux/migrate.h:5,
                    from mm/migrate.c:16:
   mm/migrate.c: In function 'migrate_pages':
>> include/linux/compiler.h:188:26: warning: 'newpage' may be used uninitialized in this function [-Wmaybe-uninitialized]
     case 8: *(__u64 *)res = *(volatile __u64 *)p; break;  \
                             ^
   mm/migrate.c:1170:15: note: 'newpage' was declared here
     struct page *newpage;
                  ^~~~~~~

vim +/newpage +188 include/linux/compiler.h

230fa253df6352 Christian Borntraeger 2014-11-25  181  
d976441f44bc5d Andrey Ryabinin       2015-10-19  182  #define __READ_ONCE_SIZE						\
d976441f44bc5d Andrey Ryabinin       2015-10-19  183  ({									\
d976441f44bc5d Andrey Ryabinin       2015-10-19  184  	switch (size) {							\
d976441f44bc5d Andrey Ryabinin       2015-10-19  185  	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;		\
d976441f44bc5d Andrey Ryabinin       2015-10-19  186  	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;		\
d976441f44bc5d Andrey Ryabinin       2015-10-19  187  	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;		\
d976441f44bc5d Andrey Ryabinin       2015-10-19 @188  	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;		\
d976441f44bc5d Andrey Ryabinin       2015-10-19  189  	default:							\
d976441f44bc5d Andrey Ryabinin       2015-10-19  190  		barrier();						\
d976441f44bc5d Andrey Ryabinin       2015-10-19  191  		__builtin_memcpy((void *)res, (const void *)p, size);	\
d976441f44bc5d Andrey Ryabinin       2015-10-19  192  		barrier();						\
d976441f44bc5d Andrey Ryabinin       2015-10-19  193  	}								\
d976441f44bc5d Andrey Ryabinin       2015-10-19  194  })
d976441f44bc5d Andrey Ryabinin       2015-10-19  195  

:::::: The code at line 188 was first introduced by commit
:::::: d976441f44bc5d48635d081d277aa76556ffbf8b compiler, atomics, kasan: Provide READ_ONCE_NOCHECK()

:::::: TO: Andrey Ryabinin <aryabinin@virtuozzo.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48332 bytes --]