Message-ID: <bx3pshg4iwcgbzihgsoxmpubbsecgm5r2x37g3sfriloke7fk3@kbyponznh3sl>
Date: Thu, 30 Oct 2025 16:52:26 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Peter Xu <peterx@...hat.com>
Cc: 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

* 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;


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

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

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

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.

Thanks,
Liam

Powered by blists - more mailing lists