On Mon, Mar 20, 2023 at 1:10 PM David Marchand
<david.march...@redhat.com> wrote:
>
> On Tue, Feb 28, 2023 at 9:45 PM David Marchand
> <david.march...@redhat.com> wrote:
> >
> > On Tue, Feb 28, 2023 at 6:29 PM Ferruh Yigit <ferruh.yi...@amd.com> wrote:
> > >
> > > KNI calls `get_user_pages_remote()` API which is using `FOLL_TOUCH`
> > > flag, but `FOLL_TOUCH` is no more in public headers since v6.3,
> > > causing a build error.
> >
> > Something looks strange with what kni was doing.
> >
> > Looking at get_user_pages_remote implementation, I see it internally
> > passes FOLL_TOUCH in addition to passed gup_flags.
> > And looking at FOLL_TOUCH definition, it seems natural (to me) that
> > this flag would be handled internally.
> >
> > Maybe it changed over time... but then the question is when did
> > passing FOLL_TOUCH become unneeded?
>
> Here is some more info.
>
> get_user_pages_remote() was added in kernel commit 1e9877902dc7
> ("mm/gup: Introduce get_user_pages_remote()").
> At this time, it was passing the FOLL_TOUCH flag internally.
>
> +long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
> + unsigned long start, unsigned long nr_pages,
> + int write, int force, struct page **pages,
> + struct vm_area_struct **vmas)
> {
> return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
> - pages, vmas, NULL, false, FOLL_TOUCH);
> + pages, vmas, NULL, false,
> + FOLL_TOUCH | FOLL_REMOTE);
> +}
> +EXPORT_SYMBOL(get_user_pages_remote);
>
> get_user_pages_remote() later gained the ability to pass gup flags in
> kernel commit 9beae1ea8930 ("mm: replace get_user_pages_remote()
> write/force parameters with gup_flags").
> But FOLL_TOUCH was still added internally.
>
> long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
> unsigned long start, unsigned long nr_pages,
> - int write, int force, struct page **pages,
> + unsigned int gup_flags, struct page **pages,
> struct vm_area_struct **vmas)
> {
> - unsigned int flags = FOLL_TOUCH | FOLL_REMOTE;
> -
> - if (write)
> - flags |= FOLL_WRITE;
> - if (force)
> - flags |= FOLL_FORCE;
> -
> return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas,
> - NULL, false, flags);
> + NULL, false,
> + gup_flags | FOLL_TOUCH | FOLL_REMOTE);
> }
>
>
> There were other changes in this area of the kernel code, but I did
> not notice a change in relation with FOLL_TOUCH.
>
> So I think the dpdk commit e73831dc6c26 ("kni: support userspace VA")
> uselessly introduced call to this flag and we can remove it.
> Adding author and reviewers of this change.
Alternatively, we could go with passing 0 in flags when FOLL_TOUCH is
not exported.
Something like:
diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h
index 7aa6cd9fca..3164a48971 100644
--- a/kernel/linux/kni/compat.h
+++ b/kernel/linux/kni/compat.h
@@ -129,6 +129,14 @@
*/
#if KERNEL_VERSION(4, 10, 0) <= LINUX_VERSION_CODE
#define HAVE_IOVA_TO_KVA_MAPPING_SUPPORT
+
+/*
+ * get_user_pages_remote() should not require the flag FOLL_TOUCH to be passed.
+ * Simply pass it as 0 when this flag is internal and not exported anymore.
+ */
+#ifndef FOLL_TOUCH
+#define FOLL_TOUCH 0
+#endif
#endif
#if KERNEL_VERSION(5, 6, 0) <= LINUX_VERSION_CODE || \
--
David Marchand
