Re: [PATCH v4 0/4] mm/userfaultfd: modulize memory types

[Peter Xu <peterx@redhat.com>]

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(-)

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

- Much less exported functions to modules

  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.

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

- Safer

  Your code allows to operate on pmd* in a module??? That's too risky and
  mm can explode!  Isn't it?

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

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

-- 
Peter Xu