On 3/20/2023 1:01 PM, David Marchand wrote:
> 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 || \
>
>
In KNI, 'get_user_pages_remote()' API is called after kernel version
4.10 and after that point 'FOLL_TOUCH' is set internally anyway as you
highlighted.
So I think we can remove setting 'FOLL_TOUCH' for all cases instead of
defining it internally, I have sent v2 according this.
