Re: [PATCH 3/5] userfualtfd: Replace lru_cache functions with folio_add functions

From: Axel Rasmussen <axelrasmussen@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>,
	"Vishal Moola (Oracle)" <vishal.moola@gmail.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org,
	Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH 3/5] userfualtfd: Replace lru_cache functions with folio_add functions
Date: Thu, 3 Nov 2022 10:34:38 -0700	[thread overview]
Message-ID: <CAJHvVcj-j6EWm5vQ74Uv1YWHbmg6-BP0hOEO2L9jRADJPEwb1A@mail.gmail.com> (raw)
In-Reply-To: <Y2LWonzCdWkDwyyr@x1n>

On Wed, Nov 2, 2022 at 1:44 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Nov 02, 2022 at 07:21:19PM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 02, 2022 at 03:02:35PM -0400, Peter Xu wrote:
> > > Does the patch attached look reasonable to you?
> >
> > Mmm, no.  If the page is in the swap cache, this will be "true".
>
> It will not happen in practise, right?
>
> I mean, shmem_get_folio() should have done the swap-in, and we should have
> the page lock held at the meantime.
>
> For anon, mcopy_atomic_pte() is the only user and it's passing in a newly
> allocated page here.
>
> >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index 3d0fef3980b3..650ab6cfd5f4 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -64,7 +64,7 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
> > >     pte_t _dst_pte, *dst_pte;
> > >     bool writable = dst_vma->vm_flags & VM_WRITE;
> > >     bool vm_shared = dst_vma->vm_flags & VM_SHARED;
> > > -   bool page_in_cache = page->mapping;
> > > +   bool page_in_cache = page_mapping(page);
> >
> > We could do:
> >
> >       struct page *head = compound_head(page);
> >       bool page_in_cache = head->mapping && !PageMappingFlags(head);
>
> Sounds good to me, but it just gets a bit complicated.
>
> If page_mapping() doesn't sound good, how about we just pass that over from
> callers?  We only have three, so quite doable too.

For what it's worth, I think I like Matthew's version better than the
original patch. This is because, although page_mapping() looks simpler
here, looking into the definition of page_mapping() I feel it's
handling several cases, not all of which are relevant here (or, as
Matthew points out, would actually be wrong if it were possible to
reach those cases here).

It's not clear to me what is meant by "pass that over from callers"?
Do you mean, have callers pass in true/false for page_in_cache
directly?

That could work, but I still think I prefer Matthew's version slightly
better, if only because this function already takes a lot of
arguments.

>
> --
> Peter Xu
>