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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

+cc CoC

Peter,

I'm sorry but your reply here is completely out of line.

I know tensions can run high sometimes, but this is a _good faith_ effort to try
to find a way forward.

Please take a step back and show some respect for the fact that Liam has put
VERY significant effort in preparing this after you _repeatedly_ asked him to
show him code.

I am starting to worry that your approach here is to bat off criticism by trying
to wear reviewers down and that's really not a good thing.

Again, this is _good faith_. Nobody is trying to unreasonably push back on these
changes, we are just trying to find the best solution possible.

Comments like:

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

and 'that's the wrong way to go. I explained to you multiple times.'

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

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

are just _entirely_ unhelpful and really unacceptable.

I have an extremely heavy workload at the moment anyway, but honestly
interactions like this have seriously put me off being involved in this review
personally.

Do we really want this to be how review in mm or the kernel is?

Is that really the culture we want to have here?

Thanks, Lorenzo


On Thu, Oct 30, 2025 at 03:55:44PM -0400, Peter Xu wrote:
> 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
>