archive mirror
 help / color / mirror / Atom feed
From: Baolin Wang <>
To: Matthew Wilcox <>
Subject: Re: [PATCH 1/5] mm: migrate: Move the page count validation to the proper place
Date: Sun, 8 Aug 2021 23:13:28 +0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.

  reply	other threads:[~2021-08-08 15:13 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-05 15:05 [PATCH 0/5] Some cleanup for page migration Baolin Wang
2021-08-05 15:05 ` [PATCH 1/5] mm: migrate: Move the page count validation to the proper place Baolin Wang
2021-08-05 15:17   ` Matthew Wilcox
2021-08-06  3:07     ` Baolin Wang
2021-08-07  2:02       ` Matthew Wilcox
2021-08-08  2:55         ` Baolin Wang
2021-08-08 10:26           ` Matthew Wilcox
2021-08-08 15:13             ` Baolin Wang [this message]
2021-08-08 16:01               ` Matthew Wilcox
2021-08-09  4:19                 ` Baolin Wang
2021-08-05 15:05 ` [PATCH 2/5] mm: migrate: Introduce a local variable to get the number of pages Baolin Wang
2021-08-05 17:42   ` Yang Shi
2021-08-05 15:05 ` [PATCH 3/5] mm: migrate: Fix the incorrect function name in comments Baolin Wang
2021-08-05 17:26   ` Yang Shi
2021-08-09 13:59   ` Alistair Popple
2021-08-05 15:05 ` [PATCH 4/5] mm: migrate: Change to use bool type for 'page_was_mapped' Baolin Wang
2021-08-05 17:34   ` Yang Shi
2021-08-05 15:06 ` [PATCH 5/5] mm: migrate: Remove redundant goto labels Baolin Wang
2021-08-05 19:54   ` Yang Shi
2021-08-06  3:20     ` Baolin Wang
2021-08-06 17:17       ` Yang Shi
2021-08-08  2:56         ` Baolin Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).