[PATCH 0/5] Some cleanup for page migration

[PATCH 0/5] Some cleanup for page migration

[Baolin Wang]

Hi,

This patch set did some cleanup and improvements for the page migration,
please help to review. Thanks a lot.

Note: the patch set is against 20210804 linux-next.

Baolin Wang (5):
  mm: migrate: Move the page count validation to the proper place
  mm: migrate: Introduce a local variable to get the number of pages
  mm: migrate: Fix the incorrect function name in comments
  mm: migrate: Change to use bool type for 'page_was_mapped'
  mm: migrate: Remove redundant goto labels

 mm/migrate.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

We've got the expected count for anonymous page or file page by
expected_page_refs() at the beginning of migrate_page_move_mapping(),
thus we should move the page count validation a little forward to
reduce duplicated code.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 239b238..5559571 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping,
 	int expected_count = expected_page_refs(mapping, &folio->page) + extra_count;
 	long nr = folio_nr_pages(folio);
 
-	if (!mapping) {
-		/* Anonymous page without mapping */
-		if (folio_ref_count(folio) != expected_count)
-			return -EAGAIN;
+	if (folio_ref_count(folio) != expected_count)
+		return -EAGAIN;
 
+	if (!mapping) {
 		/* No turning back from here */
 		newfolio->index = folio->index;
 		newfolio->mapping = folio->mapping;
@@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping,
 	newzone = folio_zone(newfolio);
 
 	xas_lock_irq(&xas);
-	if (folio_ref_count(folio) != expected_count ||
-	    xas_load(&xas) != folio) {
+	if (xas_load(&xas) != folio) {
 		xas_unlock_irq(&xas);
 		return -EAGAIN;
 	}
-- 
1.8.3.1

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Matthew Wilcox]

On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
> We've got the expected count for anonymous page or file page by
> expected_page_refs() at the beginning of migrate_page_move_mapping(),
> thus we should move the page count validation a little forward to
> reduce duplicated code.

Please add an explanation to the changelog for why it's safe to pull
this out from under the i_pages lock.

> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 239b238..5559571 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping,
>  	int expected_count = expected_page_refs(mapping, &folio->page) + extra_count;
>  	long nr = folio_nr_pages(folio);
>  
> -	if (!mapping) {
> -		/* Anonymous page without mapping */
> -		if (folio_ref_count(folio) != expected_count)
> -			return -EAGAIN;
> +	if (folio_ref_count(folio) != expected_count)
> +		return -EAGAIN;
>  
> +	if (!mapping) {
>  		/* No turning back from here */
>  		newfolio->index = folio->index;
>  		newfolio->mapping = folio->mapping;
> @@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping,
>  	newzone = folio_zone(newfolio);
>  
>  	xas_lock_irq(&xas);
> -	if (folio_ref_count(folio) != expected_count ||
> -	    xas_load(&xas) != folio) {
> +	if (xas_load(&xas) != folio) {
>  		xas_unlock_irq(&xas);
>  		return -EAGAIN;
>  	}
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

Hi Matthew,

> On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
>> We've got the expected count for anonymous page or file page by
>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>> thus we should move the page count validation a little forward to
>> reduce duplicated code.
> 
> Please add an explanation to the changelog for why it's safe to pull
> this out from under the i_pages lock.

Sure. In folio_migrate_mapping(), we are sure that the migration page 
was isolated from lru list and locked, so I think there are no race to 
get the page count without i_pages lock. Please correct me if I missed 
something else. Thanks.

> 
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/migrate.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 239b238..5559571 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping,
>>   	int expected_count = expected_page_refs(mapping, &folio->page) + extra_count;
>>   	long nr = folio_nr_pages(folio);
>>   
>> -	if (!mapping) {
>> -		/* Anonymous page without mapping */
>> -		if (folio_ref_count(folio) != expected_count)
>> -			return -EAGAIN;
>> +	if (folio_ref_count(folio) != expected_count)
>> +		return -EAGAIN;
>>   
>> +	if (!mapping) {
>>   		/* No turning back from here */
>>   		newfolio->index = folio->index;
>>   		newfolio->mapping = folio->mapping;
>> @@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping,
>>   	newzone = folio_zone(newfolio);
>>   
>>   	xas_lock_irq(&xas);
>> -	if (folio_ref_count(folio) != expected_count ||
>> -	    xas_load(&xas) != folio) {
>> +	if (xas_load(&xas) != folio) {
>>   		xas_unlock_irq(&xas);
>>   		return -EAGAIN;
>>   	}
>> -- 
>> 1.8.3.1
>>
>>

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Matthew Wilcox]

On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
> Hi Matthew,
> 
> > On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
> > > We've got the expected count for anonymous page or file page by
> > > expected_page_refs() at the beginning of migrate_page_move_mapping(),
> > > thus we should move the page count validation a little forward to
> > > reduce duplicated code.
> > 
> > Please add an explanation to the changelog for why it's safe to pull
> > this out from under the i_pages lock.
> 
> Sure. In folio_migrate_mapping(), we are sure that the migration page was
> isolated from lru list and locked, so I think there are no race to get the
> page count without i_pages lock. Please correct me if I missed something
> else. Thanks.

Unless the page has been removed from i_pages, this isn't a correct
explanation.  Even if it has been removed from i_pages, unless an
RCU grace period has passed, another CPU may still be able to inc the
refcount on it (temporarily).  The same is true for the page tables,
by the way; if someone is using get_user_pages_fast(), they may still
be able to see the page.

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

Hi,

> On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
>> Hi Matthew,
>>
>>> On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
>>>> We've got the expected count for anonymous page or file page by
>>>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>>>> thus we should move the page count validation a little forward to
>>>> reduce duplicated code.
>>>
>>> Please add an explanation to the changelog for why it's safe to pull
>>> this out from under the i_pages lock.
>>
>> Sure. In folio_migrate_mapping(), we are sure that the migration page was
>> isolated from lru list and locked, so I think there are no race to get the
>> page count without i_pages lock. Please correct me if I missed something
>> else. Thanks.
> 
> Unless the page has been removed from i_pages, this isn't a correct
> explanation.  Even if it has been removed from i_pages, unless an
> RCU grace period has passed, another CPU may still be able to inc the
> refcount on it (temporarily).  The same is true for the page tables,
> by the way; if someone is using get_user_pages_fast(), they may still
> be able to see the page.

I don't think this is an issue, cause now we've established a migration 
pte for this migration page under page lock. If the user want to get 
page by get_user_pages_fast(), it will wait for the page miggration 
finished by migration_entry_wait(). So I still think there is no need to 
check the migration page count under the i_pages lock.

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Matthew Wilcox]

On Sun, Aug 08, 2021 at 10:55:30AM +0800, Baolin Wang wrote:
> Hi,
> 
> > On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
> > > Hi Matthew,
> > > 
> > > > On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
> > > > > We've got the expected count for anonymous page or file page by
> > > > > expected_page_refs() at the beginning of migrate_page_move_mapping(),
> > > > > thus we should move the page count validation a little forward to
> > > > > reduce duplicated code.
> > > > 
> > > > Please add an explanation to the changelog for why it's safe to pull
> > > > this out from under the i_pages lock.
> > > 
> > > Sure. In folio_migrate_mapping(), we are sure that the migration page was
> > > isolated from lru list and locked, so I think there are no race to get the
> > > page count without i_pages lock. Please correct me if I missed something
> > > else. Thanks.
> > 
> > Unless the page has been removed from i_pages, this isn't a correct
> > explanation.  Even if it has been removed from i_pages, unless an
> > RCU grace period has passed, another CPU may still be able to inc the
> > refcount on it (temporarily).  The same is true for the page tables,
> > by the way; if someone is using get_user_pages_fast(), they may still
> > be able to see the page.
> 
> I don't think this is an issue, cause now we've established a migration pte
> for this migration page under page lock. If the user want to get page by
> get_user_pages_fast(), it will wait for the page miggration finished by
> migration_entry_wait(). So I still think there is no need to check the
> migration page count under the i_pages lock.

I don't know whether the patch is correct or not, but you aren't nearly
paranoid enough.  Consider this sequence of events:

CPU 0:				CPU 1:
get_user_pages_fast()
lockless_pages_from_mm()
local_irq_save()
gup_pgd_range()
gup_p4d_range()
gup_pud_range()
gup_pmd_range()
gup_pte_range()
pte_t pte = ptep_get_lockless(ptep);
				migrate_vma_collect_pmd()
				ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl)
				ptep_get_and_clear(mm, addr, ptep);
page = pte_page(pte);
				set_pte_at(mm, addr, ptep, swp_pte);
				migrate_page_move_mapping()
head = try_grab_compound_head(page, 1, flags);

... now page's refcount is temporarily higher than it should be.  CPU 0
will notice the PTE is no longer the PTE that it used to be and drop
the reference, but in the meantime, CPU 1 can observe the higher refcount.

None of this has anything to do with the i_pages lock.  Holding it does
not protect from this race, but you need to know this kind of thing to
decide if changing how we test a page's refcount is safe or not.

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

On 2021/8/8 18:26, Matthew Wilcox wrote:
> On Sun, Aug 08, 2021 at 10:55:30AM +0800, Baolin Wang wrote:
>> Hi,
>>
>>> On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
>>>> Hi Matthew,
>>>>
>>>>> On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
>>>>>> We've got the expected count for anonymous page or file page by
>>>>>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>>>>>> thus we should move the page count validation a little forward to
>>>>>> reduce duplicated code.
>>>>>
>>>>> Please add an explanation to the changelog for why it's safe to pull
>>>>> this out from under the i_pages lock.
>>>>
>>>> Sure. In folio_migrate_mapping(), we are sure that the migration page was
>>>> isolated from lru list and locked, so I think there are no race to get the
>>>> page count without i_pages lock. Please correct me if I missed something
>>>> else. Thanks.
>>>
>>> Unless the page has been removed from i_pages, this isn't a correct
>>> explanation.  Even if it has been removed from i_pages, unless an
>>> RCU grace period has passed, another CPU may still be able to inc the
>>> refcount on it (temporarily).  The same is true for the page tables,
>>> by the way; if someone is using get_user_pages_fast(), they may still
>>> be able to see the page.
>>
>> I don't think this is an issue, cause now we've established a migration pte
>> for this migration page under page lock. If the user want to get page by
>> get_user_pages_fast(), it will wait for the page miggration finished by
>> migration_entry_wait(). So I still think there is no need to check the
>> migration page count under the i_pages lock.
> 
> I don't know whether the patch is correct or not, but you aren't nearly
> paranoid enough.  Consider this sequence of events:

Thanks for describing this scenario.

> 
> CPU 0:				CPU 1:
> get_user_pages_fast()
> lockless_pages_from_mm()
> local_irq_save()
> gup_pgd_range()
> gup_p4d_range()
> gup_pud_range()
> gup_pmd_range()
> gup_pte_range()
> pte_t pte = ptep_get_lockless(ptep);
> 				migrate_vma_collect_pmd()
> 				ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl)
> 				ptep_get_and_clear(mm, addr, ptep);
> page = pte_page(pte);
> 				set_pte_at(mm, addr, ptep, swp_pte);
> 				migrate_page_move_mapping()
> head = try_grab_compound_head(page, 1, flags);

On CPU0, after grab the page count, it will validate the PTE again. If 
swap PTE has been established for this page, it will drop the count and 
go to the slow path.
if (unlikely(pte_val(pte) != pte_val(*ptep))) {
	put_compound_head(head, 1, flags);
	goto pte_unmap;
}

So CPU1 can not observe the abnormal higher refcount in this case if I 
did not miss anything.

> ... now page's refcount is temporarily higher than it should be.  CPU 0
> will notice the PTE is no longer the PTE that it used to be and drop
> the reference, but in the meantime, CPU 1 can observe the higher refcount. >
> None of this has anything to do with the i_pages lock.  Holding it does

Yes, the i_pages lock can not guarantee anything related getting page 
count, so I think we can move this out of the i_pages lock.

> not protect from this race, but you need to know this kind of thing to
> decide if changing how we test a page's refcount is safe or not.

Yes, I will continue to check if there are some races when validating 
the page count.

Any suggestion are welcome.

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Matthew Wilcox]

On Sun, Aug 08, 2021 at 11:13:28PM +0800, Baolin Wang wrote:
> On 2021/8/8 18:26, Matthew Wilcox wrote:
> > On Sun, Aug 08, 2021 at 10:55:30AM +0800, Baolin Wang wrote:
> > > Hi,
> > > 
> > > > On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
> > > > > Hi Matthew,
> > > > > 
> > > > > > On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
> > > > > > > We've got the expected count for anonymous page or file page by
> > > > > > > expected_page_refs() at the beginning of migrate_page_move_mapping(),
> > > > > > > thus we should move the page count validation a little forward to
> > > > > > > reduce duplicated code.
> > > > > > 
> > > > > > Please add an explanation to the changelog for why it's safe to pull
> > > > > > this out from under the i_pages lock.
> > > > > 
> > > > > Sure. In folio_migrate_mapping(), we are sure that the migration page was
> > > > > isolated from lru list and locked, so I think there are no race to get the
> > > > > page count without i_pages lock. Please correct me if I missed something
> > > > > else. Thanks.
> > > > 
> > > > Unless the page has been removed from i_pages, this isn't a correct
> > > > explanation.  Even if it has been removed from i_pages, unless an
> > > > RCU grace period has passed, another CPU may still be able to inc the
> > > > refcount on it (temporarily).  The same is true for the page tables,
> > > > by the way; if someone is using get_user_pages_fast(), they may still
> > > > be able to see the page.
> > > 
> > > I don't think this is an issue, cause now we've established a migration pte
> > > for this migration page under page lock. If the user want to get page by
> > > get_user_pages_fast(), it will wait for the page miggration finished by
> > > migration_entry_wait(). So I still think there is no need to check the
> > > migration page count under the i_pages lock.
> > 
> > I don't know whether the patch is correct or not, but you aren't nearly
> > paranoid enough.  Consider this sequence of events:
> 
> Thanks for describing this scenario.
> 
> > 
> > CPU 0:				CPU 1:
> > get_user_pages_fast()
> > lockless_pages_from_mm()
> > local_irq_save()
> > gup_pgd_range()
> > gup_p4d_range()
> > gup_pud_range()
> > gup_pmd_range()
> > gup_pte_range()
> > pte_t pte = ptep_get_lockless(ptep);
> > 				migrate_vma_collect_pmd()
> > 				ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl)
> > 				ptep_get_and_clear(mm, addr, ptep);
> > page = pte_page(pte);
> > 				set_pte_at(mm, addr, ptep, swp_pte);
> > 				migrate_page_move_mapping()
> > head = try_grab_compound_head(page, 1, flags);
> 
> On CPU0, after grab the page count, it will validate the PTE again. If swap
> PTE has been established for this page, it will drop the count and go to the
> slow path.
> if (unlikely(pte_val(pte) != pte_val(*ptep))) {
> 	put_compound_head(head, 1, flags);
> 	goto pte_unmap;
> }
> 
> So CPU1 can not observe the abnormal higher refcount in this case if I did
> not miss anything.

This is a race between CPUs.  There is no synchronisation between them,
so CPU 1 can absolutely see the refcount higher temporarily.  Yes,
CPU 0 will eventually put the refcount, but CPU 1 can observe it high.

Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

On 2021/8/9 0:01, Matthew Wilcox wrote:
> On Sun, Aug 08, 2021 at 11:13:28PM +0800, Baolin Wang wrote:
>> On 2021/8/8 18:26, Matthew Wilcox wrote:
>>> On Sun, Aug 08, 2021 at 10:55:30AM +0800, Baolin Wang wrote:
>>>> Hi,
>>>>
>>>>> On Fri, Aug 06, 2021 at 11:07:18AM +0800, Baolin Wang wrote:
>>>>>> Hi Matthew,
>>>>>>
>>>>>>> On Thu, Aug 05, 2021 at 11:05:56PM +0800, Baolin Wang wrote:
>>>>>>>> We've got the expected count for anonymous page or file page by
>>>>>>>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>>>>>>>> thus we should move the page count validation a little forward to
>>>>>>>> reduce duplicated code.
>>>>>>>
>>>>>>> Please add an explanation to the changelog for why it's safe to pull
>>>>>>> this out from under the i_pages lock.
>>>>>>
>>>>>> Sure. In folio_migrate_mapping(), we are sure that the migration page was
>>>>>> isolated from lru list and locked, so I think there are no race to get the
>>>>>> page count without i_pages lock. Please correct me if I missed something
>>>>>> else. Thanks.
>>>>>
>>>>> Unless the page has been removed from i_pages, this isn't a correct
>>>>> explanation.  Even if it has been removed from i_pages, unless an
>>>>> RCU grace period has passed, another CPU may still be able to inc the
>>>>> refcount on it (temporarily).  The same is true for the page tables,
>>>>> by the way; if someone is using get_user_pages_fast(), they may still
>>>>> be able to see the page.
>>>>
>>>> I don't think this is an issue, cause now we've established a migration pte
>>>> for this migration page under page lock. If the user want to get page by
>>>> get_user_pages_fast(), it will wait for the page miggration finished by
>>>> migration_entry_wait(). So I still think there is no need to check the
>>>> migration page count under the i_pages lock.
>>>
>>> I don't know whether the patch is correct or not, but you aren't nearly
>>> paranoid enough.  Consider this sequence of events:
>>
>> Thanks for describing this scenario.
>>
>>>
>>> CPU 0:				CPU 1:
>>> get_user_pages_fast()
>>> lockless_pages_from_mm()
>>> local_irq_save()
>>> gup_pgd_range()
>>> gup_p4d_range()
>>> gup_pud_range()
>>> gup_pmd_range()
>>> gup_pte_range()
>>> pte_t pte = ptep_get_lockless(ptep);
>>> 				migrate_vma_collect_pmd()
>>> 				ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl)
>>> 				ptep_get_and_clear(mm, addr, ptep);
>>> page = pte_page(pte);
>>> 				set_pte_at(mm, addr, ptep, swp_pte);
>>> 				migrate_page_move_mapping()
>>> head = try_grab_compound_head(page, 1, flags);
>>
>> On CPU0, after grab the page count, it will validate the PTE again. If swap
>> PTE has been established for this page, it will drop the count and go to the
>> slow path.
>> if (unlikely(pte_val(pte) != pte_val(*ptep))) {
>> 	put_compound_head(head, 1, flags);
>> 	goto pte_unmap;
>> }
>>
>> So CPU1 can not observe the abnormal higher refcount in this case if I did
>> not miss anything.
> 
> This is a race between CPUs.  There is no synchronisation between them,
> so CPU 1 can absolutely see the refcount higher temporarily.  Yes,
> CPU 0 will eventually put the refcount, but CPU 1 can observe it high.

OK, I understood your concern. I agree CPU 1 can observe refcount higher 
temporarily, but the migrate_page_move_mapping() has passed the page 
count validation, and will think the page mapping can be migrated, since 
CPU0 will failed to get the page count to go to the slow path.

If the CPU0 increase the page count after page_count() validation in 
migrate_page_move_mapping() on CPU1, and CPU1 will freeze the page count 
to repalce the mapping.
if (!page_ref_freeze(page, expected_count)) {
	xas_unlock_irq(&xas);
	return -EAGAIN;
}

So CPU0 will failed to increase page count by try_grab_compound_head() 
if this page count is under freezing; or CPU1 will failed to freeze the 
page count if CPU0 increases page count successfully, which will abort 
the migration; or after the CPU1 freezing, the CPU0 will increase the 
page count successfully, but will put the page count since PTE was 
changed. Until now, I did not see any terrible things when validating 
the page count in migrate_page_move_mapping() if I understood correctly.

But I have another question, should we change to use ptep_get_lockless() 
instead of pte_val(*ptep) to validate the PTE in gup_pte_range(), to 
avoid getting the old value?
@@ -2185,7 +2185,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long 
addr, unsigned long end,
                         goto pte_unmap;
                 }

-               if (unlikely(pte_val(pte) != pte_val(*ptep))) {
+               if (unlikely(pte_val(pte) != ptep_get_lockless(ptep))) {
                         put_compound_head(head, 1, flags);
                         goto pte_unmap;
                 }

[PATCH 2/5] mm: migrate: Introduce a local variable to get the number of pages

[Baolin Wang]

Use thp_nr_pages() instead of compound_nr() to get the number of pages
for THP page, meanwhile introducing a local variable 'nr_pages' to
avoid getting the number of pages repeatedly.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 5559571..eeba4c6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2106,6 +2106,7 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
 static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
 	int page_lru;
+	int nr_pages = thp_nr_pages(page);
 
 	VM_BUG_ON_PAGE(compound_order(page) && !PageTransHuge(page), page);
 
@@ -2114,7 +2115,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 		return 0;
 
 	/* Avoid migrating to a node that is nearly full */
-	if (!migrate_balanced_pgdat(pgdat, compound_nr(page)))
+	if (!migrate_balanced_pgdat(pgdat, nr_pages))
 		return 0;
 
 	if (isolate_lru_page(page))
@@ -2122,7 +2123,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 
 	page_lru = page_is_file_lru(page);
 	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
-				thp_nr_pages(page));
+			    nr_pages);
 
 	/*
 	 * Isolating the page has taken another reference, so the
-- 
1.8.3.1

Re: [PATCH 2/5] mm: migrate: Introduce a local variable to get the number of pages

[Yang Shi]

On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Use thp_nr_pages() instead of compound_nr() to get the number of pages
> for THP page, meanwhile introducing a local variable 'nr_pages' to
> avoid getting the number of pages repeatedly.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Yang Shi <shy828301@gmail.com>

> ---
>  mm/migrate.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5559571..eeba4c6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2106,6 +2106,7 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
>  static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  {
>         int page_lru;
> +       int nr_pages = thp_nr_pages(page);
>
>         VM_BUG_ON_PAGE(compound_order(page) && !PageTransHuge(page), page);
>
> @@ -2114,7 +2115,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>                 return 0;
>
>         /* Avoid migrating to a node that is nearly full */
> -       if (!migrate_balanced_pgdat(pgdat, compound_nr(page)))
> +       if (!migrate_balanced_pgdat(pgdat, nr_pages))
>                 return 0;
>
>         if (isolate_lru_page(page))
> @@ -2122,7 +2123,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>
>         page_lru = page_is_file_lru(page);
>         mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
> -                               thp_nr_pages(page));
> +                           nr_pages);
>
>         /*
>          * Isolating the page has taken another reference, so the
> --
> 1.8.3.1
>
>

[PATCH 3/5] mm: migrate: Fix the incorrect function name in comments

[Baolin Wang]

since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
the migration ptes establishment has been split into a separate
try_to_migrate() function, thus update the related comments.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index eeba4c6..6f048a8 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 	}
 
 	/*
-	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
+	 * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
 	 * we cannot notice that anon_vma is freed while we migrates a page.
 	 * This get_anon_vma() delays freeing anon_vma pointer until the end
 	 * of migration. File cache pages are no problem because of page_lock()
-- 
1.8.3.1

Re: [PATCH 3/5] mm: migrate: Fix the incorrect function name in comments

[Yang Shi]

On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
> the migration ptes establishment has been split into a separate
> try_to_migrate() function, thus update the related comments.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Yang Shi <shy828301@gmail.com>

> ---
>  mm/migrate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index eeba4c6..6f048a8 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>         }
>
>         /*
> -        * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
> +        * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
>          * we cannot notice that anon_vma is freed while we migrates a page.
>          * This get_anon_vma() delays freeing anon_vma pointer until the end
>          * of migration. File cache pages are no problem because of page_lock()
> --
> 1.8.3.1
>
>

Re: [PATCH 3/5] mm: migrate: Fix the incorrect function name in comments

[Alistair Popple]

Thanks for catching that.

Reviewed-by: Alistair Popple <apopple@nvidia.com>

On Friday, 6 August 2021 1:05:58 AM AEST Baolin Wang wrote:
> since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
> the migration ptes establishment has been split into a separate
> try_to_migrate() function, thus update the related comments.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index eeba4c6..6f048a8 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>  	}
>  
>  	/*
> -	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
> +	 * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
>  	 * we cannot notice that anon_vma is freed while we migrates a page.
>  	 * This get_anon_vma() delays freeing anon_vma pointer until the end
>  	 * of migration. File cache pages are no problem because of page_lock()
> 

[PATCH 4/5] mm: migrate: Change to use bool type for 'page_was_mapped'

[Baolin Wang]

Change to use bool type for 'page_was_mapped' variable making it
more readable.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 6f048a8..0ab364f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -957,7 +957,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 				int force, enum migrate_mode mode)
 {
 	int rc = -EAGAIN;
-	int page_was_mapped = 0;
+	bool page_was_mapped = false;
 	struct anon_vma *anon_vma = NULL;
 	bool is_lru = !__PageMovable(page);
 
@@ -1060,7 +1060,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma,
 				page);
 		try_to_migrate(page, 0);
-		page_was_mapped = 1;
+		page_was_mapped = true;
 	}
 
 	if (!page_mapped(page))
-- 
1.8.3.1

Re: [PATCH 4/5] mm: migrate: Change to use bool type for 'page_was_mapped'

[Yang Shi]

On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Change to use bool type for 'page_was_mapped' variable making it
> more readable.

Seems better to me. Reviewed-by: Yang Shi <shy828301@gmail.com>

>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6f048a8..0ab364f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -957,7 +957,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>                                 int force, enum migrate_mode mode)
>  {
>         int rc = -EAGAIN;
> -       int page_was_mapped = 0;
> +       bool page_was_mapped = false;
>         struct anon_vma *anon_vma = NULL;
>         bool is_lru = !__PageMovable(page);
>
> @@ -1060,7 +1060,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>                 VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma,
>                                 page);
>                 try_to_migrate(page, 0);
> -               page_was_mapped = 1;
> +               page_was_mapped = true;
>         }
>
>         if (!page_mapped(page))
> --
> 1.8.3.1
>
>

[PATCH 5/5] mm: migrate: Remove redundant goto labels

[Baolin Wang]

Remove redundant goto labels to simplify the code.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 0ab364f..ed74fda 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		 */
 		VM_BUG_ON_PAGE(!PageIsolated(page), page);
 		if (!PageMovable(page)) {
-			rc = MIGRATEPAGE_SUCCESS;
 			__ClearPageIsolated(page);
-			goto out;
+			return MIGRATEPAGE_SUCCESS;
 		}
 
 		rc = mapping->a_ops->migratepage(mapping, newpage,
@@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 			flush_dcache_page(newpage);
 
 	}
-out:
+
 	return rc;
 }
 
@@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
 	newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
 				   HPAGE_PMD_ORDER);
 	if (!newpage)
-		goto out;
+		return NULL;
 
 	prep_transhuge_page(newpage);
 
-out:
 	return newpage;
 }
 
-- 
1.8.3.1

Re: [PATCH 5/5] mm: migrate: Remove redundant goto labels

[Yang Shi]

On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Remove redundant goto labels to simplify the code.

TBH I don't see too much benefit. The "goto" makes the functions have
a single exit point.

>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 0ab364f..ed74fda 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>                  */
>                 VM_BUG_ON_PAGE(!PageIsolated(page), page);
>                 if (!PageMovable(page)) {
> -                       rc = MIGRATEPAGE_SUCCESS;
>                         __ClearPageIsolated(page);
> -                       goto out;
> +                       return MIGRATEPAGE_SUCCESS;
>                 }
>
>                 rc = mapping->a_ops->migratepage(mapping, newpage,
> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>                         flush_dcache_page(newpage);
>
>         }
> -out:
> +
>         return rc;
>  }
>
> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
>         newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
>                                    HPAGE_PMD_ORDER);
>         if (!newpage)
> -               goto out;
> +               return NULL;
>
>         prep_transhuge_page(newpage);
>
> -out:
>         return newpage;
>  }
>
> --
> 1.8.3.1
>
>

Re: [PATCH 5/5] mm: migrate: Remove redundant goto labels

[Baolin Wang]

Hi Yang,

> On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>> Remove redundant goto labels to simplify the code.
> 
> TBH I don't see too much benefit. The "goto" makes the functions have
> a single exit point.

Yes, I agree that the 'goto' statement can make things easier when a 
function exits from multiple locations and some common work such as 
cleanup has to be done, as well as introducing complexity to reading the 
code. So per the coding style documentation, "If there is no cleanup 
needed then just return directly", which can make code more readable I 
think :)

But I have no strong opinion on this, I can drop this patch if you still 
think this is unnecessary. Thanks for your review and comments.

>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/migrate.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 0ab364f..ed74fda 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>>                   */
>>                  VM_BUG_ON_PAGE(!PageIsolated(page), page);
>>                  if (!PageMovable(page)) {
>> -                       rc = MIGRATEPAGE_SUCCESS;
>>                          __ClearPageIsolated(page);
>> -                       goto out;
>> +                       return MIGRATEPAGE_SUCCESS;
>>                  }
>>
>>                  rc = mapping->a_ops->migratepage(mapping, newpage,
>> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>>                          flush_dcache_page(newpage);
>>
>>          }
>> -out:
>> +
>>          return rc;
>>   }
>>
>> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
>>          newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
>>                                     HPAGE_PMD_ORDER);
>>          if (!newpage)
>> -               goto out;
>> +               return NULL;
>>
>>          prep_transhuge_page(newpage);
>>
>> -out:
>>          return newpage;
>>   }
>>
>> --
>> 1.8.3.1
>>
>>

Re: [PATCH 5/5] mm: migrate: Remove redundant goto labels

[Yang Shi]

On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Hi Yang,
>
> > On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
> > <baolin.wang@linux.alibaba.com> wrote:
> >>
> >> Remove redundant goto labels to simplify the code.
> >
> > TBH I don't see too much benefit. The "goto" makes the functions have
> > a single exit point.
>
> Yes, I agree that the 'goto' statement can make things easier when a
> function exits from multiple locations and some common work such as
> cleanup has to be done, as well as introducing complexity to reading the
> code. So per the coding style documentation, "If there is no cleanup
> needed then just return directly", which can make code more readable I
> think :)
>
> But I have no strong opinion on this, I can drop this patch if you still
> think this is unnecessary. Thanks for your review and comments.

Thanks, IMHO I'd like to drop it for now.

>
> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> >> ---
> >>   mm/migrate.c | 8 +++-----
> >>   1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index 0ab364f..ed74fda 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -911,9 +911,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >>                   */
> >>                  VM_BUG_ON_PAGE(!PageIsolated(page), page);
> >>                  if (!PageMovable(page)) {
> >> -                       rc = MIGRATEPAGE_SUCCESS;
> >>                          __ClearPageIsolated(page);
> >> -                       goto out;
> >> +                       return MIGRATEPAGE_SUCCESS;
> >>                  }
> >>
> >>                  rc = mapping->a_ops->migratepage(mapping, newpage,
> >> @@ -949,7 +948,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
> >>                          flush_dcache_page(newpage);
> >>
> >>          }
> >> -out:
> >> +
> >>          return rc;
> >>   }
> >>
> >> @@ -2095,11 +2094,10 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
> >>          newpage = alloc_pages_node(nid, (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
> >>                                     HPAGE_PMD_ORDER);
> >>          if (!newpage)
> >> -               goto out;
> >> +               return NULL;
> >>
> >>          prep_transhuge_page(newpage);
> >>
> >> -out:
> >>          return newpage;
> >>   }
> >>
> >> --
> >> 1.8.3.1
> >>
> >>

Re: [PATCH 5/5] mm: migrate: Remove redundant goto labels

[Baolin Wang]

> On Thu, Aug 5, 2021 at 8:19 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>> Hi Yang,
>>
>>> On Thu, Aug 5, 2021 at 8:06 AM Baolin Wang
>>> <baolin.wang@linux.alibaba.com> wrote:
>>>>
>>>> Remove redundant goto labels to simplify the code.
>>>
>>> TBH I don't see too much benefit. The "goto" makes the functions have
>>> a single exit point.
>>
>> Yes, I agree that the 'goto' statement can make things easier when a
>> function exits from multiple locations and some common work such as
>> cleanup has to be done, as well as introducing complexity to reading the
>> code. So per the coding style documentation, "If there is no cleanup
>> needed then just return directly", which can make code more readable I
>> think :)
>>
>> But I have no strong opinion on this, I can drop this patch if you still
>> think this is unnecessary. Thanks for your review and comments.
> 
> Thanks, IMHO I'd like to drop it for now.

OK, will do. Thanks.