Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[David Hildenbrand]

On 03.10.23 19:05, Suren Baghdasaryan wrote:
> On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > Let's clean up do_wp_page() a bit, removing two labels and making it
> > a easier to read.
> >
> > wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
> > SetPageAnonExclusive() out into do_wp_page(). No need to do this under
> > page lock -- the page table lock is sufficient.
> >
> > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > ---
> >   mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
> >   1 file changed, 45 insertions(+), 43 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 1f0e3317cbdd..512f6f05620e 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
> >          return ret;
> >   }

Sorry for the late response.

> > +static bool wp_can_reuse_anon_folio(struct folio *folio,
> > +                                   struct vm_area_struct *vma)
>
> Since this function is calling folio_move_anon_rmap(), I would suggest
> changing its name to wp_reuse_anon_folio(). This would clarify that

folio_move_anon_rmap() is *not* the reuse part, it's just an rmap 
optimization. You could remove the call here and the whole thing would 
still work :) In fact, we can call folio_move_anon_rmap() whenever we 
sure the folio belongs to a single VMA only and we're holding the page 
lock. ... but we cannot always reuse a folio in that case because there 
might be GUP references from another process.

Reuse is
* Setting PageAnonExclusive
* Write fault: wunprotect the page -> wp_page_reuse()

I went a bit back and forth while cleaning that function up, but calling 
it wp_reuse_anon_folio() would end up being confusing with 
wp_page_reuse() called afterwards [we should probably rename that one to 
wp_page_wunprotect() independently]. So I prefer to leave the actual 
(sub)page reuse part in the caller.

> it's actually doing that operation instead of just checking if it's
> possible. That would also let us keep unconditional
> SetPageAnonExclusive() in it and do that under folio lock like it used
> to do (keeping rules simple). Other than that, it looks good to me.

I really want to avoid passing a "struct page" to that function; once 
we're dealing with PTE-mapped THP, the page might actually be a tail 
page of the folio.

Thanks!

--
Cheers,

David / dhildenb