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

On Sun, Jun 29, 2025 at 11:50:11AM +0300, Mike Rapoport wrote:
> Hi Peter,

Hi, Mike,

> 
> On Fri, Jun 27, 2025 at 11:46:52AM -0400, Peter Xu wrote:
> > Introduce a generic userfaultfd API for vm_operations_struct, so that one
> > vma, especially when as a module, can support userfaults without modifying
> > the core files.  More importantly, when the module can be compiled out of
> > the kernel.
> > 
> > So, instead of having core mm referencing modules that may not ever exist,
> > we need to have modules opt-in on core mm hooks instead.
> > 
> > After this API applied, if a module wants to support userfaultfd, the
> > module should only need to touch its own file and properly define
> > vm_uffd_ops, instead of changing anything in core mm.
> 
> I liked the changelog update you proposed in v1 thread. I took liberty to

It's definitely hard to satisfy all reviewers on one version of commit
message..

> slightly update it and here's what I've got:
> 
>   Currently, most of the userfaultfd features are implemented directly in the
>   core mm.  It will invoke VMA specific functions whenever necessary.  So far
>   it is fine because it almost only interacts with shmem and hugetlbfs.
> 
>   Introduce a generic userfaultfd API extension for vm_operations_struct,
>   so that any code that implements vm_operations_struct (including kernel
>   modules that can be compiled separately from the kernel core) can support
>   userfaults without modifying the core files.
> 
>   With this API applied, if a module wants to support userfaultfd, the
>   module should only need to properly define vm_uffd_ops and hook it to
>   vm_operations_struct, instead of changing anything in core mm.

Thanks, I very much appreciate explicit suggestions on the wordings.
Personally I like it and the rest suggestions, I'll use it when repost, but
I'll also wait for others if anyone has other things to say.

> 
> > Note that such API will not work for anonymous. Core mm will process
> > anonymous memory separately for userfault operations like before.
> 
> Maybe:
> 
>   This API will not work for anonymous memory. Handling of userfault
>   operations for anonymous memory remains unchanged in core mm.
>  
> > This patch only introduces the API alone so that we can start to move
> > existing users over but without breaking them.
> 
> Please use imperative mood, e.g.
>  
>   Only introduce the new API so that ...
>  
> > Currently the uffd_copy() API is almost designed to be the simplistic with
> > minimum mm changes to move over to the API.
> > 
> > Signed-off-by: Peter Xu <peterx@...hat.com>
> > ---
> >  include/linux/mm.h            |  9 ++++++
> >  include/linux/userfaultfd_k.h | 52 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index ef40f68c1183..6a5447bd43fd 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -576,6 +576,8 @@ struct vm_fault {
> >  					 */
> >  };
> >  
> > +struct vm_uffd_ops;
> > +
> >  /*
> >   * These are the virtual MM functions - opening of an area, closing and
> >   * unmapping it (needed to keep files on disk up-to-date etc), pointer
> > @@ -653,6 +655,13 @@ struct vm_operations_struct {
> >  	 */
> >  	struct page *(*find_special_page)(struct vm_area_struct *vma,
> >  					  unsigned long addr);
> > +#ifdef CONFIG_USERFAULTFD
> > +	/*
> > +	 * Userfaultfd related ops.  Modules need to define this to support
> > +	 * userfaultfd.
> > +	 */
> > +	const struct vm_uffd_ops *userfaultfd_ops;
> > +#endif
> >  };
> >  
> >  #ifdef CONFIG_NUMA_BALANCING
> > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > index df85330bcfa6..c9a093c4502b 100644
> > --- a/include/linux/userfaultfd_k.h
> > +++ b/include/linux/userfaultfd_k.h
> > @@ -92,6 +92,58 @@ enum mfill_atomic_mode {
> >  	NR_MFILL_ATOMIC_MODES,
> >  };
> >  
> > +/* VMA userfaultfd operations */
> > +struct vm_uffd_ops {
> > +	/**
> > +	 * @uffd_features: features supported in bitmask.
> > +	 *
> > +	 * When the ops is defined, the driver must set non-zero features
> > +	 * to be a subset (or all) of: VM_UFFD_MISSING|WP|MINOR.
> > +	 */
> > +	unsigned long uffd_features;
> > +	/**
> > +	 * @uffd_ioctls: ioctls supported in bitmask.
> > +	 *
> > +	 * Userfaultfd ioctls supported by the module.  Below will always
> > +	 * be supported by default whenever a module provides vm_uffd_ops:
> > +	 *
> > +	 *   _UFFDIO_API, _UFFDIO_REGISTER, _UFFDIO_UNREGISTER, _UFFDIO_WAKE
> > +	 *
> > +	 * The module needs to provide all the rest optionally supported
> > +	 * ioctls.  For example, when VM_UFFD_MISSING was supported,
> > +	 * _UFFDIO_COPY must be supported as ioctl, while _UFFDIO_ZEROPAGE
> > +	 * is optional.
> > +	 */
> > +	unsigned long uffd_ioctls;
> > +	/**
> > +	 * uffd_get_folio: Handler to resolve UFFDIO_CONTINUE request.
> > +	 *
> > +	 * @inode: the inode for folio lookup
> > +	 * @pgoff: the pgoff of the folio
> > +	 * @folio: returned folio pointer
> > +	 *
> > +	 * Return: zero if succeeded, negative for errors.
> > +	 */
> > +	int (*uffd_get_folio)(struct inode *inode, pgoff_t pgoff,
> > +			      struct folio **folio);
> > +	/**
> > +	 * uffd_copy: Handler to resolve UFFDIO_COPY|ZEROPAGE request.
> > +	 *
> > +	 * @dst_pmd: target pmd to resolve page fault
> > +	 * @dst_vma: target vma
> > +	 * @dst_addr: target virtual address
> > +	 * @src_addr: source address to copy from
> > +	 * @flags: userfaultfd request flags
> > +	 * @foliop: previously allocated folio
> > +	 *
> > +	 * Return: zero if succeeded, negative for errors.
> > +	 */
> > +	int (*uffd_copy)(pmd_t *dst_pmd, struct vm_area_struct *dst_vma,
> > +			 unsigned long dst_addr, unsigned long src_addr,
> > +			 uffd_flags_t flags, struct folio **foliop);
> > +};
> > +typedef struct vm_uffd_ops vm_uffd_ops;
> 
> Either use vm_uffd_ops_t for the typedef or drop the typedef entirely. My
> preference is for the second option.

Andrew helped me to fix some hidden spaces which I appreciated, then I
found checkpatch warns on this one too besides the spaces fixed in mm-new.
I do not know why checkpatch doesn't like typedefs even if typedefs are
massively used in Linux..

I think I'll simply stick with not using typedefs.

Thanks,

> 
> > +
> >  #define MFILL_ATOMIC_MODE_BITS (const_ilog2(NR_MFILL_ATOMIC_MODES - 1) + 1)
> >  #define MFILL_ATOMIC_BIT(nr) BIT(MFILL_ATOMIC_MODE_BITS + (nr))
> >  #define MFILL_ATOMIC_FLAG(nr) ((__force uffd_flags_t) MFILL_ATOMIC_BIT(nr))
> > -- 
> > 2.49.0
> > 
> 
> -- 
> Sincerely yours,
> Mike.
> 

-- 
Peter Xu

Powered by blists - more mailing lists