[PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Axel Rasmussen]

For background, mm/userfaultfd.c provides a general mcopy_atomic
implementation. But some types of memory (e.g., hugetlb and shmem) need
a slightly different implementation, so they provide their own helpers
for this. In other words, userfaultfd is the only caller of this
function.

This patch achieves two things:

1. Don't spend time compiling code which will end up never being
referenced anyway (a small build time optimization).

2. In future patches (e.g. [1]), we plan to extend the signature of
these helpers with UFFD-specific state (e.g., enums or structs defined
conditionally in userfaultfd_k.h). Once this happens, this patch will be
needed to avoid build errors (or, we'd need to define more UFFD-only
stuff unconditionally, which seems messier to me).

Peter Xu suggested this be sent as a standalone patch, in the mailing
list discussion for [1].

[1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091

Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
---
 include/linux/hugetlb.h | 4 ++++
 mm/hugetlb.c            | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index ebca2ef02212..749701b5c153 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -134,11 +134,13 @@ void hugetlb_show_meminfo(void);
 unsigned long hugetlb_total_pages(void);
 vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 			unsigned long address, unsigned int flags);
+#ifdef CONFIG_USERFAULTFD
 int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, pte_t *dst_pte,
 				struct vm_area_struct *dst_vma,
 				unsigned long dst_addr,
 				unsigned long src_addr,
 				struct page **pagep);
+#endif
 int hugetlb_reserve_pages(struct inode *inode, long from, long to,
 						struct vm_area_struct *vma,
 						vm_flags_t vm_flags);
@@ -308,6 +310,7 @@ static inline void hugetlb_free_pgd_range(struct mmu_gather *tlb,
 	BUG();
 }
 
+#ifdef CONFIG_USERFAULTFD
 static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 						pte_t *dst_pte,
 						struct vm_area_struct *dst_vma,
@@ -318,6 +321,7 @@ static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	BUG();
 	return 0;
 }
+#endif
 
 static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr,
 					unsigned long sz)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 18f6ee317900..821bfa9c0c80 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4615,6 +4615,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	return ret;
 }
 
+#ifdef CONFIG_USERFAULTFD
 /*
  * Used by userfaultfd UFFDIO_COPY.  Based on mcopy_atomic_pte with
  * modifications for huge pages.
@@ -4745,6 +4746,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	put_page(page);
 	goto out;
 }
+#endif
 
 long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 			 struct page **pages, struct vm_area_struct **vmas,
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Peter Xu]

On Tue, Feb 02, 2021 at 12:31:27PM -0800, Axel Rasmussen wrote:
> For background, mm/userfaultfd.c provides a general mcopy_atomic
> implementation. But some types of memory (e.g., hugetlb and shmem) need
> a slightly different implementation, so they provide their own helpers
> for this. In other words, userfaultfd is the only caller of this
> function.
> 
> This patch achieves two things:
> 
> 1. Don't spend time compiling code which will end up never being
> referenced anyway (a small build time optimization).
> 
> 2. In future patches (e.g. [1]), we plan to extend the signature of
> these helpers with UFFD-specific state (e.g., enums or structs defined
> conditionally in userfaultfd_k.h). Once this happens, this patch will be
> needed to avoid build errors (or, we'd need to define more UFFD-only
> stuff unconditionally, which seems messier to me).
> 
> Peter Xu suggested this be sent as a standalone patch, in the mailing
> list discussion for [1].
> 
> [1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>

I meant a standalone patch along with the next version of your series would be
good enough... :) If Mike is fine I won't complain if you'd squashed it into
that patch either.  The patch itself looks correct to me.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Hugh Dickins]

On Tue, 2 Feb 2021, Axel Rasmussen wrote:

> For background, mm/userfaultfd.c provides a general mcopy_atomic
> implementation. But some types of memory (e.g., hugetlb and shmem) need
> a slightly different implementation, so they provide their own helpers
> for this. In other words, userfaultfd is the only caller of this
> function.
> 
> This patch achieves two things:
> 
> 1. Don't spend time compiling code which will end up never being
> referenced anyway (a small build time optimization).
> 
> 2. In future patches (e.g. [1]), we plan to extend the signature of
> these helpers with UFFD-specific state (e.g., enums or structs defined
> conditionally in userfaultfd_k.h). Once this happens, this patch will be
> needed to avoid build errors (or, we'd need to define more UFFD-only
> stuff unconditionally, which seems messier to me).
> 
> Peter Xu suggested this be sent as a standalone patch, in the mailing
> list discussion for [1].
> 
> [1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  include/linux/hugetlb.h | 4 ++++
>  mm/hugetlb.c            | 2 ++
>  2 files changed, 6 insertions(+)

Hi Axel, please also do the same to mm/shmem.c (perhaps you missed
it because I did that long ago to our internal copy of mm/shmem.c).
But please also comment the endifs
#endif /* CONFIG_USERFAULTFD */
to help find one's way around them.

I see you've done include/linux/hugetlb.h: okay, that's not necessary,
but a matter of taste; up to you whether to do include/linux/shmem_fs.h.

Thanks,
Hugh

> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index ebca2ef02212..749701b5c153 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -134,11 +134,13 @@ void hugetlb_show_meminfo(void);
>  unsigned long hugetlb_total_pages(void);
>  vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  			unsigned long address, unsigned int flags);
> +#ifdef CONFIG_USERFAULTFD
>  int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, pte_t *dst_pte,
>  				struct vm_area_struct *dst_vma,
>  				unsigned long dst_addr,
>  				unsigned long src_addr,
>  				struct page **pagep);
> +#endif
>  int hugetlb_reserve_pages(struct inode *inode, long from, long to,
>  						struct vm_area_struct *vma,
>  						vm_flags_t vm_flags);
> @@ -308,6 +310,7 @@ static inline void hugetlb_free_pgd_range(struct mmu_gather *tlb,
>  	BUG();
>  }
>  
> +#ifdef CONFIG_USERFAULTFD
>  static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  						pte_t *dst_pte,
>  						struct vm_area_struct *dst_vma,
> @@ -318,6 +321,7 @@ static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  	BUG();
>  	return 0;
>  }
> +#endif
>  
>  static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr,
>  					unsigned long sz)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 18f6ee317900..821bfa9c0c80 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4615,6 +4615,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_USERFAULTFD
>  /*
>   * Used by userfaultfd UFFDIO_COPY.  Based on mcopy_atomic_pte with
>   * modifications for huge pages.
> @@ -4745,6 +4746,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  	put_page(page);
>  	goto out;
>  }
> +#endif
>  
>  long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  			 struct page **pages, struct vm_area_struct **vmas,
> -- 
> 2.30.0.365.g02bc693789-goog

Re: [PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Axel Rasmussen]

On Tue, Feb 2, 2021 at 1:03 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Tue, 2 Feb 2021, Axel Rasmussen wrote:
>
> > For background, mm/userfaultfd.c provides a general mcopy_atomic
> > implementation. But some types of memory (e.g., hugetlb and shmem) need
> > a slightly different implementation, so they provide their own helpers
> > for this. In other words, userfaultfd is the only caller of this
> > function.
> >
> > This patch achieves two things:
> >
> > 1. Don't spend time compiling code which will end up never being
> > referenced anyway (a small build time optimization).
> >
> > 2. In future patches (e.g. [1]), we plan to extend the signature of
> > these helpers with UFFD-specific state (e.g., enums or structs defined
> > conditionally in userfaultfd_k.h). Once this happens, this patch will be
> > needed to avoid build errors (or, we'd need to define more UFFD-only
> > stuff unconditionally, which seems messier to me).
> >
> > Peter Xu suggested this be sent as a standalone patch, in the mailing
> > list discussion for [1].
> >
> > [1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> > ---
> >  include/linux/hugetlb.h | 4 ++++
> >  mm/hugetlb.c            | 2 ++
> >  2 files changed, 6 insertions(+)
>
> Hi Axel, please also do the same to mm/shmem.c (perhaps you missed
> it because I did that long ago to our internal copy of mm/shmem.c).

I had been largely ignoring shmem up to this point because my minor
fault handling series doesn't (yet) deal with it. But, I'll need to do
this later when I support shmem anyway, so happy to add it here.

> But please also comment the endifs
> #endif /* CONFIG_USERFAULTFD */
> to help find one's way around them.

Done. :)

>
> I see you've done include/linux/hugetlb.h: okay, that's not necessary,
> but a matter of taste; up to you whether to do include/linux/shmem_fs.h.

Ah, so it is not strictly needed in the current world, but later when
the signature contains UFFD-specific things like the mode enumeration
I'm proposing, it becomes necessary, or else we get many build
warnings about implicitly declaring the argument's type.

Sorry for misunderstanding Peter's recommendation; this would be more
apparent if this patch was part of the larger series. Perhaps the
right thing to do is just abandon this separate patch and move it back
into the larger series (keeping your suggestions, of course). I
suppose I'll do that - you'll see this in v4 of
https://patchwork.kernel.org/project/linux-mm/list/?series=424091
unless someone tells me to do otherwise. :P

Thanks for reviewing, all.

>
> Thanks,
> Hugh
>
> >
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index ebca2ef02212..749701b5c153 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -134,11 +134,13 @@ void hugetlb_show_meminfo(void);
> >  unsigned long hugetlb_total_pages(void);
> >  vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> >                       unsigned long address, unsigned int flags);
> > +#ifdef CONFIG_USERFAULTFD
> >  int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, pte_t *dst_pte,
> >                               struct vm_area_struct *dst_vma,
> >                               unsigned long dst_addr,
> >                               unsigned long src_addr,
> >                               struct page **pagep);
> > +#endif
> >  int hugetlb_reserve_pages(struct inode *inode, long from, long to,
> >                                               struct vm_area_struct *vma,
> >                                               vm_flags_t vm_flags);
> > @@ -308,6 +310,7 @@ static inline void hugetlb_free_pgd_range(struct mmu_gather *tlb,
> >       BUG();
> >  }
> >
> > +#ifdef CONFIG_USERFAULTFD
> >  static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >                                               pte_t *dst_pte,
> >                                               struct vm_area_struct *dst_vma,
> > @@ -318,6 +321,7 @@ static inline int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >       BUG();
> >       return 0;
> >  }
> > +#endif
> >
> >  static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr,
> >                                       unsigned long sz)
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 18f6ee317900..821bfa9c0c80 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -4615,6 +4615,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> >       return ret;
> >  }
> >
> > +#ifdef CONFIG_USERFAULTFD
> >  /*
> >   * Used by userfaultfd UFFDIO_COPY.  Based on mcopy_atomic_pte with
> >   * modifications for huge pages.
> > @@ -4745,6 +4746,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >       put_page(page);
> >       goto out;
> >  }
> > +#endif
> >
> >  long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
> >                        struct page **pages, struct vm_area_struct **vmas,
> > --
> > 2.30.0.365.g02bc693789-goog

Re: [PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Hugh Dickins]

On Tue, 2 Feb 2021, Axel Rasmussen wrote:
> On Tue, Feb 2, 2021 at 1:03 PM Hugh Dickins <hughd@google.com> wrote:
> > On Tue, 2 Feb 2021, Axel Rasmussen wrote:
> >
> > > For background, mm/userfaultfd.c provides a general mcopy_atomic
> > > implementation. But some types of memory (e.g., hugetlb and shmem) need
> > > a slightly different implementation, so they provide their own helpers
> > > for this. In other words, userfaultfd is the only caller of this
> > > function.
> > >
> > > This patch achieves two things:
> > >
> > > 1. Don't spend time compiling code which will end up never being
> > > referenced anyway (a small build time optimization).
> > >
> > > 2. In future patches (e.g. [1]), we plan to extend the signature of
> > > these helpers with UFFD-specific state (e.g., enums or structs defined
> > > conditionally in userfaultfd_k.h). Once this happens, this patch will be
> > > needed to avoid build errors (or, we'd need to define more UFFD-only
> > > stuff unconditionally, which seems messier to me).
> > >
> > > Peter Xu suggested this be sent as a standalone patch, in the mailing
> > > list discussion for [1].
> > >
> > > [1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091
> > >
> > > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> > > ---
> > >  include/linux/hugetlb.h | 4 ++++
> > >  mm/hugetlb.c            | 2 ++
> > >  2 files changed, 6 insertions(+)
> >
> > Hi Axel, please also do the same to mm/shmem.c (perhaps you missed
> > it because I did that long ago to our internal copy of mm/shmem.c).
> 
> I had been largely ignoring shmem up to this point because my minor
> fault handling series doesn't (yet) deal with it. But, I'll need to do
> this later when I support shmem anyway, so happy to add it here.

Oh, if this patch is going into a hugetlbfs series, skip mm/shmem.c for
now (or keep it in, whichever's easiest for you): I caught sight of the
"(e.g., hugetlb and shmem)" in the commit message above, and thought
you had inadvertently missed out the shmem part - but now see that
the patch title does say "userfaultfd: hugetlbfs:".

Hugh

Re: [PATCH] userfaultfd: hugetlbfs: only compile UFFD helpers if config enabled

[Mike Kravetz]

On 2/2/21 12:31 PM, Axel Rasmussen wrote:
> For background, mm/userfaultfd.c provides a general mcopy_atomic
> implementation. But some types of memory (e.g., hugetlb and shmem) need
> a slightly different implementation, so they provide their own helpers
> for this. In other words, userfaultfd is the only caller of this
> function.
> 
> This patch achieves two things:
> 
> 1. Don't spend time compiling code which will end up never being
> referenced anyway (a small build time optimization).
> 
> 2. In future patches (e.g. [1]), we plan to extend the signature of
> these helpers with UFFD-specific state (e.g., enums or structs defined
> conditionally in userfaultfd_k.h). Once this happens, this patch will be
> needed to avoid build errors (or, we'd need to define more UFFD-only
> stuff unconditionally, which seems messier to me).
> 
> Peter Xu suggested this be sent as a standalone patch, in the mailing
> list discussion for [1].
> 
> [1] https://patchwork.kernel.org/project/linux-mm/list/?series=424091
> 
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  include/linux/hugetlb.h | 4 ++++
>  mm/hugetlb.c            | 2 ++
>  2 files changed, 6 insertions(+)

When you move this back into the "userfaultfd: add minor fault handling"
series, feel free to add:

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Thanks,
-- 
Mike Kravetz