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

[Suren Baghdasaryan]

On Mon, Oct 9, 2023 at 3:03 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> 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()

Ok, fair enough. It's not the reuse, only a preparation step. My
concern is that wp_can_reuse_anon_folio() with a bool being returned
looks like a function that only checks for a possibility of an
operation while it's doing more than that. However I don't have a
really good suggestion to improve the naming, so treat it as a nitpick
and feel free to ignore.

>
> 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.

Oh, I didn't realize that vmf->page and folio->page might differ in
here. Ok, sounds reasonable.
Thanks,
Suren.

>
> Thanks!
>
> --
> Cheers,
>
> David / dhildenb
>