On Jun 21, 2022, at 1:48 AM, David Hildenbrand <david@xxxxxxxxxx> wrote:
> ⚠ External Email
>
> On 20.06.22 01:34, Nadav Amit wrote:
>> From: Nadav Amit <namit@xxxxxxxxxx>
>>
>> Using a PTE on x86 with cleared access-bit (aka young-bit)
>> takes ~600 cycles more than when the access bit is set. At the same
>> time, setting the access-bit for memory that is not used (e.g.,
>> prefetched) can introduce greater overheads, as the prefetched memory is
>> reclaimed later than it should be.
>>
>> Userfaultfd currently does not set the access-bit (excluding the
>> huge-pages case). Arguably, it is best to let the user control whether
>> the access bit should be set or not. The expected use is to request
>> userfaultfd to set the access-bit when the copy/wp operation is done to
>> resolve a page-fault, and not to set the access-bit when the memory is
>> prefetched.
>>
>> Introduce UFFDIO_COPY_MODE_ACCESS_LIKELY and
>> UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY to enable userspace to request
>> the young bit to be set. Set for UFFDIO_CONTINUE and UFFDIO_ZEROPAGE the
>> bit unconditionally since the former is only used to resolve page-faults
>> and the latter would not benefit from not setting the access-bit.
>>
>> Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
>> Cc: Peter Xu <peterx@xxxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
>> Signed-off-by: Nadav Amit <namit@xxxxxxxxxx>
>> ---
>> fs/userfaultfd.c | 23 ++++++++++++++++-------
>> include/linux/userfaultfd_k.h | 1 +
>> include/uapi/linux/userfaultfd.h | 20 +++++++++++++++++++-
>> mm/userfaultfd.c | 18 ++++++++++++++++--
>> 4 files changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
>> index 5daafa54eb3f..35a8c4347c54 100644
>> --- a/fs/userfaultfd.c
>> +++ b/fs/userfaultfd.c
>> @@ -1700,7 +1700,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
>> struct uffdio_copy uffdio_copy;
>> struct uffdio_copy __user *user_uffdio_copy;
>> struct userfaultfd_wake_range range;
>> - bool mode_wp;
>> + bool mode_wp, mode_access_likely;
>> uffd_flags_t uffd_flags;
>>
>> user_uffdio_copy = (struct uffdio_copy __user *) arg;
>> @@ -1726,12 +1726,15 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
>> ret = -EINVAL;
>> if (uffdio_copy.src + uffdio_copy.len <= uffdio_copy.src)
>> goto out;
>> - if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP))
>> + if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP|
>> + UFFDIO_COPY_MODE_ACCESS_LIKELY))
>> goto out;
>>
>> mode_wp = uffdio_copy.mode & UFFDIO_COPY_MODE_WP;
>> + mode_access_likely = uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY;
>
> I *relly* prefer just
>
> if (uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY)
> uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY
> [...]
>
>> - uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0);
>> + uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0) |
>> + (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0);
>
> Dito.
>
>> if (mmget_not_zero(ctx->mm)) {
>> ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
>> @@ -1871,6 +1877,7 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
>> struct uffdio_continue uffdio_continue;
>> struct uffdio_continue __user *user_uffdio_continue;
>> struct userfaultfd_wake_range range;
>> + uffd_flags_t uffd_flags;
>>
>> user_uffdio_continue = (struct uffdio_continue __user *)arg;
>>
>> @@ -1898,10 +1905,12 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
>> if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
>> goto out;
>>
>> + uffd_flags = UFFD_FLAGS_ACCESS_LIKELY;
>
> Can we add a comment why that makes sense? I think I know why -- someone
> is stuck waiting for that continue to happen :)
>
>> +
>> if (mmget_not_zero(ctx->mm)) {
>> ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,
>> uffdio_continue.range.len,
>> - &ctx->mmap_changing, 0);
>> + &ctx->mmap_changing, uffd_flags);
>> mmput(ctx->mm);
>> } else {
>> return -ESRCH;
>> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
>> index 6331148023c1..e6ac165ec044 100644
>> --- a/include/linux/userfaultfd_k.h
>> +++ b/include/linux/userfaultfd_k.h
>> @@ -58,6 +58,7 @@ enum mcopy_atomic_mode {
>> typedef unsigned int __bitwise uffd_flags_t;
>>
>> #define UFFD_FLAGS_WP ((__force uffd_flags_t)BIT(0))
>> +#define UFFD_FLAGS_ACCESS_LIKELY ((__force uffd_flags_t)BIT(1))
>>
>> extern int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
>> struct vm_area_struct *dst_vma,
>> diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
>> index 005e5e306266..d9c8ce9ba777 100644
>> --- a/include/uapi/linux/userfaultfd.h
>> +++ b/include/uapi/linux/userfaultfd.h
>> @@ -38,7 +38,8 @@
>> UFFD_FEATURE_MINOR_HUGETLBFS | \
>> UFFD_FEATURE_MINOR_SHMEM | \
>> UFFD_FEATURE_EXACT_ADDRESS | \
>> - UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
>> + UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \
>> + UFFD_FEATURE_ACCESS_HINTS)
>> #define UFFD_API_IOCTLS \
>> ((__u64)1 << _UFFDIO_REGISTER | \
>> (__u64)1 << _UFFDIO_UNREGISTER | \
>> @@ -203,6 +204,10 @@ struct uffdio_api {
>> *
>> * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
>> * write-protection mode is supported on both shmem and hugetlbfs.
>> + *
>> + * UFFD_FEATURE_ACCESS_HINTS indicates that the copy supports
>> + * UFFDIO_COPY_MODE_ACCESS_LIKELY supports
>> + * UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY.
>
> I think that sentence doesn't make sense.
English… :)
How about
* UFFD_FEATURE_ACCESS_HINTS indicates that the ioctl operations
* supports the UFFDIO_*_MODE_[ACCESS|WRITE]_LIKELY and
* UFFDIO_*_MODE_[ACCESS|WRITE]_LIKELY hints.
But that would mean that for consistency, I would need to provide
zero/continue hints (which might be disregarded by the kernel)?
>> */
>> #define UFFD_FEATURE_PAGEFAULT_FLAG_WP (1<<0)
>> #define UFFD_FEATURE_EVENT_FORK (1<<1)
>> @@ -217,6 +222,7 @@ struct uffdio_api {
>> #define UFFD_FEATURE_MINOR_SHMEM (1<<10)
>> #define UFFD_FEATURE_EXACT_ADDRESS (1<<11)
>> #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM (1<<12)
>> +#define UFFD_FEATURE_ACCESS_HINTS (1<<13)
>> __u64 features;
>>
>> __u64 ioctls;
>> @@ -260,6 +266,13 @@ struct uffdio_copy {
>> * copy_from_user will not read the last 8 bytes.
>> */
>> __s64 copy;
>> + /*
>> + * UFFDIO_COPY_MODE_ACCESS_LIKELY will set the mapped page as young.
>
> Setting the page young is an implementation detail. Can you phrase it
> more generically what the effect of that hint might be?
Err. I forgot to fix it before sending. How about:
* UFFDIO_COPY_MODE_ACCESS_LIKELY provides a hint to the kernel
* that the page is likely to be access in the near future. Providing
* the hint properly can improve performance.
?
>
>> @@ -691,6 +699,9 @@ ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,
>> unsigned long len, atomic_t *mmap_changing,
>> uffd_flags_t uffd_flags)
>> {
>> + /* There is no cost for setting the access bit of a zeropage */
>> + uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
>> +
>> return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_ZEROPAGE,
>> mmap_changing, 0);
>> }
>> @@ -699,6 +710,9 @@ ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,
>> unsigned long len, atomic_t *mmap_changing,
>> uffd_flags_t uffd_flags)
>> {
>> + /* The page is likely to be accessed */
>> + uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
>
> Shoouldn't that be set by the caller already?
I thought that it belongs conceptually to mm/userfaultfd and not
fs/userfaultfd.
I will wait for Axel input as to how to handle the CONTINUE case and fix it
accordingly.
