Message-ID: <aQPZqyUUVjexKWaJ@x1.local>
Date: Thu, 30 Oct 2025 17:33:31 -0400
From: Peter Xu <peterx@...hat.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
	David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Mike Rapoport <rppt@...nel.org>,
	Muchun Song <muchun.song@...ux.dev>,
	Nikita Kalyazin <kalyazin@...zon.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	Axel Rasmussen <axelrasmussen@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	James Houghton <jthoughton@...gle.com>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Hugh Dickins <hughd@...gle.com>, Michal Hocko <mhocko@...e.com>,
	Ujwal Kundur <ujwal.kundur@...il.com>,
	Oscar Salvador <osalvador@...e.de>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH v4 0/4] mm/userfaultfd: modulize memory types

On Thu, Oct 30, 2025 at 04:52:26PM -0400, Liam R. Howlett wrote:
> * Peter Xu <peterx@...hat.com> [251030 15:56]:
> > On Thu, Oct 30, 2025 at 03:07:18PM -0400, Peter Xu wrote:
> > > > Patches are here:
> > > > 
> > > > https://git.infradead.org/?p=users/jedix/linux-maple.git;a=shortlog;h=refs/heads/modularized_mem
> > > 
> > > Great!  Finally we have something solid to discuss on top.
> > > 
> > > Yes, I'm extremely happy to see whatever code there is, I'm happy to review
> > > it.  I'm happy to see it rolling.  If it is better, we can adopt it.
> > 
> > So here is a summary of why I think my proposal is better:
> > 
> > - Much less code
> > 
> >   I think this is crystal clear..  I'm pasting once more in this summary
> >   email on what your proposal touches:
> > 
> >  fs/userfaultfd.c              |  14 +--
> >  include/linux/hugetlb.h       |  21 ----
> >  include/linux/mm.h            |  11 ++
> >  include/linux/shmem_fs.h      |  14 ---
> >  include/linux/userfaultfd_k.h | 108 ++++++++++------
> >  mm/hugetlb.c                  | 359 +++++++++++++++++++++++++++++++++++++++++++++--------
> >  mm/shmem.c                    | 245 ++++++++++++++++++++++++------------
> >  mm/userfaultfd.c              | 869 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------
> >  8 files changed, 962 insertions(+), 679 deletions(-)
> 
> Including the following highlights:
>  -#include <linux/hugetlb.h>
> 
> and
> 
>  -typedef unsigned int __bitwise uffd_flags_t;

So you spent ~1000 LOC for these as highlights..  It's ok, I'll wait for a
second opinion.

> 
> 
> > 
> > - Much less future code
> > 
> >   The new proposal needs at least 6 APIs to implement even minor fault..
> >   One of the API needs to be implemented with uffd_info* which further
> >   includes 10+ fields to process.  It means we'll have a bunch of
> >   duplicated code in the future if new things pop up, so it's not only
> >   about what we merge.
> 
> You can reuse existing functions if there is no change.
> 
> > 
> > - Much less exported functions to modules
> 
> I haven't exported anything.  You asked for code and I provided it.
> This doesn't do what guest_memfd needs as it is.  This is all clean up
> you wouldn't do.
> 
> > 
> >   My solution, after exposing vm_uffd_ops, doesn't need to export any
> >   function.
> > 
> >   Your solution needs to export a lot of new functions to modules.  I
> >   didn't pay a lot of attention but the list should at least include these
> >   10 functions:
> >   
> >         void uffd_complete_register(struct vm_area_struct *vma);
> >         unsigned int uffd_page_shift(struct vm_area_struct *vma);
> >         int uffd_writeprotect(struct uffd_info *info);
> >         ssize_t uffd_failed_do_unlock(struct uffd_info *info);
> >         int uffd_atomic_pte_copy(struct folio *folio, unsigned long src_addr);
> >         unsigned long mfill_size(struct vm_area_struct *vma)
> >         int mfill_atomic_pte_poison(struct uffd_info *info);
> >         int mfill_atomic_pte_copy(struct uffd_info *info);
> >         int mfill_atomic_pte_zeropage(struct uffd_info *info);
> >         ssize_t uffd_get_dst_pmd(struct vm_area_struct *dst_vma, unsigned long dst_addr,pmd_t **dst_pmd);
> > 
> >   It's simply unnecessary.
> 
> Maybe we don't export any of them.  Maybe there's another function
> pointer that could be checked or overwritten?  We can do that without a
> flag or setting or wahtever the name you used for your flag was.

You need to export them when guest-memfd will be involved, am I right?

> 
> > 
> > - Less error prone
> > 
> >   At least to support minor fault, my solution only needs one hook fetching
> >   page cache, then set the CONTINUE ioctl in the supported_ioctls.
> 
> Your code just adds more junk to uffd, and fails to modularize anything
> beyond getting a folio.  And you only support certain types and places.

So would CoC accepts "junk" in this context?

> 
> > 
> > - Safer
> > 
> >   Your code allows to operate on pmd* in a module??? That's too risky and
> >   mm can explode!  Isn't it?
> 
> Again. I didn't export anything.
> 
> > 
> > - Do not build new codes on top of hugetlbfs
> > 
> >   AFAICT, more than half of your solution's API is trying to service
> >   hugetlbfs.  IMHO that's the wrong way to go.  I explained to you multiple
> >   times.  We should either keep hugetlbfs alone, or having hugetlbfs adopt
> >   mm APIs instead.  We shouldn't build any new code only trying to service
> >   hugetlbfsv1 but nobody else.  We shouldn't introduce new mm API only to
> >   service hugetlbfs.
> 
> Ignoring hugetlb exists is a problem.  I have removed the hugetlb from
> being included in uffd while you have left it in its own loop.  This
> doesn't build new things on hugetlb, it moves the code for hugetlb out
> of mm/userfaultfd.c - how is it building on hugetlb?

The APIs you introduced is building for hugetlb.  If without hugetlb, more
than half of the API is not needed.

> 
> Believe it or not, hugetlb is a memory type.
> 
> Certainly smaller changes are inherently less bug prone.  I honestly
> think all of what I have here needs to be done, regardless of memfd
> support.  I cannot believe that things were allowed to be pushed this
> far.  I do not think they should be allowed to go further.
> 
> > 
> > - Much less risk of breaking things
> > 
> >   I'm pretty sure my code introduce zero or very little bug, if there's
> >   one, I'll fix it, but really, likely not, because the changes are
> >   straightforward.
> > 
> >   Your changes are huge.  I would not be surprised you break things here
> >   and there.  I hope at least you will be around fixing them when it
> >   happens, even if we're not sure the benefits of most of the changes.
> 
> I have always been prompt in fixing my issues and have taken
> responsibility for anything I've written.  I maintain the maple tree and
> other areas of mm.  I have no plans of leaving Linux and I hope not to
> die.
> 
> I can maintain mm/userfaultfd.c, if that helps.  I didn't feel like I
> knew the area enough before, but I'm learning a lot doing this.

I'm not maintainer of userfaultfd, I'm a reviewer.  I was almost just
trying to help, in reality that is also true that obviously I don't make
decisions.

I definitely think you can at least propose add yourself as a reviewer if
you like to start looking after userfaultfds, or even M if Andrew likes it.

You're already listed as core mm R so I don't see much difference if it's
only a R addition.

-- 
Peter Xu

Powered by blists - more mailing lists