Message-ID: <9ce3aaaa-71a6-5a81-16a3-36e6763feb91@redhat.com>
Date:   Tue, 30 Aug 2022 20:53:06 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...e.de>,
        John Hubbard <jhubbard@...dia.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Hugh Dickins <hughd@...gle.com>, Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH v1 2/3] mm/gup: use gup_can_follow_protnone() also in
 GUP-fast

On 30.08.22 20:45, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2022 at 08:23:52PM +0200, David Hildenbrand wrote:
>> ... and looking into the details of TLB flush and GUP-fast interaction
>> nowadays, that case is no longer relevant. A TLB flush is no longer
>> sufficient to stop concurrent GUP-fast ever since we introduced generic
>> RCU GUP-fast.
> 
> Yes, we've had RCU GUP fast for a while, and it is more widely used
> now, IIRC.
> 
> It has been a bit, but if I remember, GUP fast in RCU mode worked on a
> few principles:
> 
>  - The PTE page must not be freed without RCU
>  - The PTE page content must be convertable to a struct page using the
>    usual rules (eg PTE Special)
>  - That struct page refcount may go from 0->1 inside the RCU
>  - In the case the refcount goes from 0->1 there must be sufficient
>    barriers such that GUP fast observing the refcount of 1 will also
>    observe the PTE entry has changed. ie before the refcount is
>    dropped in the zap it has to clear the PTE entry, the refcount
>    decr has to be a 'release' and the refcount incr in gup fast has be
>    to be an 'acquire'.
>  - The rest of the system must tolerate speculative refcount
>    increments from GUP on any random page
> > The basic idea being that if GUP fast obtains a valid reference on a
> page *and* the PTE entry has not changed then everything is fine.
> 
> The tricks with TLB invalidation are just a "poor mans" RCU, and
> arguably these days aren't really needed since I think we could make
> everything use real RCU always without penalty if we really wanted.
> 
> Today we can create a unique 'struct pagetable_page' as Matthew has
> been doing in other places that guarentees a rcu_head is always
> available for every page used in a page table. Using that we could
> drop the code in the TLB flusher that allocates memory for the
> rcu_head and hopes for the best. (Or even is the common struct page
> rcu_head already guarenteed to exist for pagetable pages now a days?)
> 
> IMHO that is the main reason we still have the non-RCU mode at all..


Good, I managed to attract the attention of someone who understands that machinery :)

While validating whether GUP-fast and PageAnonExclusive code work correctly,
I started looking at the whole RCU GUP-fast machinery. I do have a patch to
improve PageAnonExclusive clearing (I think we're missing memory barriers to
make it work as expected in any possible case), but I also stumbled eventually
over a more generic issue that might need memory barriers.

Any thoughts whether I am missing something or this is actually missing
memory barriers?


>From ce8c941c11d1f60cea87a3e4d941041dc6b79900 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@...hat.com>
Date: Mon, 29 Aug 2022 16:57:07 +0200
Subject: [PATCH] mm/gup: update refcount+pincount before testing if the PTE
 changed

mm/ksm.c:write_protect_page() has to make sure that no unknown
references to a mapped page exist and that no additional ones with write
permissions are possible -- unknown references could have write permissions
and modify the page afterwards.

Conceptually, mm/ksm.c:write_protect_page() consists of:
  (1) Clear/invalidate PTE
  (2) Check if there are unknown references; back off if so.
  (3) Update PTE (e.g., map it R/O)

Conceptually, GUP-fast code consists of:
  (1) Read the PTE
  (2) Increment refcount/pincount of the mapped page
  (3) Check if the PTE changed by re-reading it; back off if so.

To make sure GUP-fast won't be able to grab additional references after
clearing the PTE, but will properly detect the change and back off, we
need a memory barrier between updating the recount/pincount and checking
if it changed.

try_grab_folio() doesn't necessarily imply a memory barrier, so add an
explicit smp_mb__after_atomic() after the atomic RMW operation to
increment the refcount and pincount.

ptep_clear_flush() used to clear the PTE and flush the TLB should imply
a memory barrier for flushing the TLB, so don't add another one for now.

PageAnonExclusive handling requires further care and will be handled
separately.

Fixes: 2667f50e8b81 ("mm: introduce a general RCU get_user_pages_fast()")
Signed-off-by: David Hildenbrand <david@...hat.com>
---
 mm/gup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index 5abdaf487460..0008b808f484 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2392,6 +2392,14 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
 			goto pte_unmap;
 		}
 
+		/*
+		 * Update refcount/pincount before testing for changed PTE. This
+		 * is required for code like mm/ksm.c:write_protect_page() that
+		 * wants to make sure that a page has no unknown references
+		 * after clearing the PTE.
+		 */
+		smp_mb__after_atomic();
+
 		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
 			gup_put_folio(folio, 1, flags);
 			goto pte_unmap;
@@ -2577,6 +2585,9 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,
 	if (!folio)
 		return 0;
 
+	/* See gup_pte_range(). */
+	smp_mb__after_atomic();
+
 	if (unlikely(pte_val(pte) != pte_val(*ptep))) {
 		gup_put_folio(folio, refs, flags);
 		return 0;
@@ -2643,6 +2654,9 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 	if (!folio)
 		return 0;
 
+	/* See gup_pte_range(). */
+	smp_mb__after_atomic();
+
 	if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
 		gup_put_folio(folio, refs, flags);
 		return 0;
@@ -2683,6 +2697,9 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
 	if (!folio)
 		return 0;
 
+	/* See gup_pte_range(). */
+	smp_mb__after_atomic();
+
 	if (unlikely(pud_val(orig) != pud_val(*pudp))) {
 		gup_put_folio(folio, refs, flags);
 		return 0;
-- 
2.37.1




-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists