On Mon, Feb 01, 2021 at 02:11:55PM -0800, Axel Rasmussen wrote:

On Mon, Feb 1, 2021 at 11:21 AM Peter Xu [off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 02:48:17PM -0800, Axel Rasmussen wrote:

quoted

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index f94a35296618..79e1f0155afa 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h

@@ -135,11 +135,14 @@ 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

I'm confused why this is needed.. hugetlb_mcopy_atomic_pte() should only be
called in userfaultfd.c, but if without uffd config set it won't compile
either:

        obj-$(CONFIG_USERFAULTFD) += userfaultfd.o

With this series as-is, but *without* the #ifdef CONFIG_USERFAULTFD
here, we introduce a bunch of build warnings like this:



In file included from ./include/linux/migrate.h:8:0,
                 from kernel/sched/sched.h:53,
                 from kernel/sched/isolation.c:10:
./include/linux/hugetlb.h:143:12: warning: 'enum mcopy_atomic_mode'
declared inside parameter list
     struct page **pagep);
            ^
./include/linux/hugetlb.h:143:12: warning: its scope is only this
definition or declaration, which is probably not what you want

And similarly we get an error about the "mode" parameter having an
incomplete type in hugetlb.c.



This is because enum mcopy_atomic_mode is defined in userfaultfd_k.h,
and that entire header is wrapped in a #ifdef CONFIG_USERFAULTFD. So
we either need to define enum mcopy_atomic_mode unconditionally, or we
need to #ifdef CONFIG_USERFAULTFD the references to it also.

- I opted not to move it outside the #ifdef CONFIG_USERFAULTFD in
userfaultfd_k.h (defining it unconditionally), because that seemed
messy to me.
- I opted not to define it unconditionally in hugetlb.h, because we'd
have to move it to userfaultfd_k.h anyway when shmem or other users
are introduced. I'm planning to send a series to add this a few days
or so after this series is merged, so it seems churn-y to move it
then.
- It seemed optimal to not compile hugetlb_mcopy_atomic_pte anyway
(even ignoring adding the continue ioctl), since as you point out
userfaultfd is the only caller.

Hopefully this clarifies this and the next two comments. Let me know
if you still feel strongly, I don't hate any of the alternatives, just
wanted to clarify that I had considered them and thought this approach
was best.

Then I'd suggest you use a standalone patch to put hugetlb_mcopy_atomic_pte()
into CONFIG_USERFAULTFD blocks, then propose your change with the minor mode.
Note that there're two hugetlb_mcopy_atomic_pte() defined in hugetlb.h.
Although I don't think it a problem since the other one is inlined - I think
you should still put that one into the same ifdef:

#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,
						unsigned long dst_addr,
						unsigned long src_addr,
						struct page **pagep)
{
	BUG();
	return 0;
}
#endif /* CONFIG_USERFAULTFD */

Let's also see whether Mike would have a preference on this.

Thanks,

-- 
Peter Xu