userfaultfd: remove one unnecessary warn_on in __mcopy_atomic_hugetlb
Subsystems (2)
PATCH 1/2 userfaultfd: remove one unnecessary warn_on in __mcopy_atomic_hugetlb
The warning here is to make sure address(dst_addr) and length(len - copied) are huge page size aligned.
While this is ensured by:
dst_start and len is huge page size aligned
dst_addr equals to dst_start and increase huge page size each time copied increase huge page size each time
This means this warning will never be triggered.
--- mm/userfaultfd.c | 4 ---- 1 file changed, 4 deletions(-)
mm/userfaultfd.c +0 -4
@@ -243,10 +243,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, vm_shared = dst_vma->vm_flags &VM_SHARED ; } -if (WARN_ON (dst_addr & (vma_hpagesize -1 ) || - (len - copied) & (vma_hpagesize -1 ))) - goto out_unlock; -/* * If not shared, ensure the dst_vma has a anon_vma. */
Signature · 1 line
2.17.1
PATCH 2/2 userfaultfd: wrap the common dst_vma check into an inlined function
When doing UFFDIO_COPY, it is necessary to find the correct destination vma and make sure fault range is in it.
Since there are two places need to do the same task, just wrap those common check into an inlined function.
--- mm/userfaultfd.c | 56 +++++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 24 deletions(-)
mm/userfaultfd.c +32 -24
@@ -18,6 +18,36 @@#include <asm/tlbflush.h> #include "internal.h" +static __always_inline +struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm, +unsigned long dst_start, +unsigned long len) +{ +/* +* Make sure that the dst range is both valid and fully within a +* single existing vma. +*/ +struct vm_area_struct *dst_vma; + + dst_vma =find_vma (dst_mm, dst_start); +if (!dst_vma) +return NULL ; + +if (dst_start < dst_vma->vm_start || + dst_start + len > dst_vma->vm_end) +return NULL ; + +/* +* Check the vma is registered in uffd, this is required to +* enforce the VM_MAYWRITE check done at uffd registration +* time. +*/ +if (!dst_vma->vm_userfaultfd_ctx.ctx) +return NULL ; + +return dst_vma; +} + static int mcopy_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, struct vm_area_struct *dst_vma,
@@ -221,20 +251,9 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,*/ if (!dst_vma) { err = -ENOENT ; - dst_vma =find_vma (dst_mm, dst_start); + dst_vma =find_dst_vma (dst_mm, dst_start, len);if (!dst_vma || !is_vm_hugetlb_page (dst_vma)) goto out_unlock; -/* -* Check the vma is registered in uffd, this is -* required to enforce the VM_MAYWRITE check done at -* uffd registration time. -*/ -if (!dst_vma->vm_userfaultfd_ctx.ctx) - goto out_unlock; - -if (dst_start < dst_vma->vm_start || - dst_start + len > dst_vma->vm_end) - goto out_unlock; err = -EINVAL ;if (vma_hpagesize !=vma_kernel_pagesize (dst_vma))
@@ -471,20 +490,9 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,* both valid and fully within a single existing vma. */ err = -ENOENT ; - dst_vma =find_vma (dst_mm, dst_start); + dst_vma =find_dst_vma (dst_mm, dst_start, len);if (!dst_vma) goto out_unlock; -/* -* Check the vma is registered in uffd, this is required to -* enforce the VM_MAYWRITE check done at uffd registration -* time. -*/ -if (!dst_vma->vm_userfaultfd_ctx.ctx) - goto out_unlock; - -if (dst_start < dst_vma->vm_start || - dst_start + len > dst_vma->vm_end) - goto out_unlock; err = -EINVAL ;/*
Signature · 1 line
2.17.1
Quoted 8 lines · Wei Yang
The warning here is to make sure
address(dst_addr)and length(len - copied) are huge page size aligned.While this is ensured by:
dst_startand len is huge page size aligned
dst_addrequals todst_startand increase huge page size each time copied increase huge page size each time
Can we also remove the following for the same reasons?
mm/userfaultfd.c +0 -1
@@ -262,7 +262,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,pte_t dst_pteval;BUG_ON (dst_addr >= dst_start + len); -VM_BUG_ON (dst_addr & ~huge_page_mask (h));/* * Serialize via hugetlb_fault_mutex
Signature · 1 line
Mike Kravetz
Quoted 24 lines
This means this warning will never be triggered.
Signed-off-by Wei Yang <[email protected]>--- mm/userfaultfd.c | 4 ---- 1 file changed, 4 deletions(-)mm/userfaultfd.c +0
-4@@ -243,10 +243,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, vm_shared = dst_vma->vm_flags &VM_SHARED ; } -if (WARN_ON (dst_addr & (vma_hpagesize -1 ) || - (len - copied) & (vma_hpagesize -1 ))) - goto out_unlock; -/* * If not shared, ensure the dst_vma has a anon_vma. */
Quoted 17 lines · Mike Kravetz
Quoted 8 lines · Wei Yang
The warning here is to make sure
address(dst_addr)and length(len - copied) are huge page size aligned.While this is ensured by:
dst_startand len is huge page size aligned
dst_addrequals todst_startand increase huge page size each time copied increase huge page size each timeCan we also remove the following for the same reasons?
mm/userfaultfd.c +0
-0
mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 265
Quoted 5 lines
mm/userfaultfd.c +0
-1@@ -262,7 +262,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,pte_t dst_pteval;BUG_ON (dst_addr >= dst_start + len); -VM_BUG_ON (dst_addr & ~huge_page_mask (h));
Thanks for your comment.
It looks good, while I lack some knowledge between vma_hpagesize and huge_page_mask().
If they are the same, why not use the same interface for all those checks in this function?
Quoted 30 lines
mm/userfaultfd.c +0
-0/* * Serialize via hugetlb_fault_mutex Signature · 2 lines
-- Mike KravetzQuoted 24 lines
This means this warning will never be triggered.
Signed-off-by Wei Yang <[email protected]>--- mm/userfaultfd.c | 4 ---- 1 file changed, 4 deletions(-)mm/userfaultfd.c +0
-4@@ -243,10 +243,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, vm_shared = dst_vma->vm_flags &VM_SHARED ; } -if (WARN_ON (dst_addr & (vma_hpagesize -1 ) || - (len - copied) & (vma_hpagesize -1 ))) - goto out_unlock; -/* * If not shared, ensure the dst_vma has a anon_vma. */
Signature · 2 lines
Wei Yang Help you, Help me
Quoted 20 lines · Wei Yang
Quoted 19 lines · Mike Kravetz
Quoted 8 lines · Wei Yang
The warning here is to make sure
address(dst_addr)and length(len - copied) are huge page size aligned.While this is ensured by:
dst_startand len is huge page size aligned
dst_addrequals todst_startand increase huge page size each time copied increase huge page size each timeCan we also remove the following for the same reasons?
mm/userfaultfd.c +0
-0@@ -262,7 +262,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,pte_t dst_pteval;
Quoted 8 lines
mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 265
Quoted 3 lines
mm/userfaultfd.c +0
-1BUG_ON (dst_addr >= dst_start + len); -VM_BUG_ON (dst_addr & ~huge_page_mask (h));Thanks for your comment.
It looks good, while I lack some knowledge between
vma_hpagesizeandhuge_page_mask().
vma_hpagesize is just a local variable used so that repeated calls to vma_kernel_pagesize() or huge_page_size() are not necessary.
Quoted 2 lines
If they are the same, why not use the same interface for all those checks in this function?
If we remove the VM_BUG_ON, that is the only use of huge_page_mask() in the function.
We can can also eliminate a call to huge_page_size() by making this change.
@@ -273,7 +272,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
mutex_lock(&hugetlb_fault_mutex_table[hash]);
err = -ENOMEM;
- dst_pte = huge_pte_alloc(dst_mm, dst_addr, huge_page_size(h));
+ dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize);
if (!dst_pte) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]); goto out_unlock;
Signature · 1 line
Mike Kravetz
Quoted 27 lines · Mike Kravetz
Quoted 26 lines · Wei Yang
Quoted 17 lines · Mike Kravetz
Quoted 8 lines · Wei Yang
The warning here is to make sure
address(dst_addr)and length(len - copied) are huge page size aligned.While this is ensured by:
dst_startand len is huge page size aligned
dst_addrequals todst_startand increase huge page size each time copied increase huge page size each timeCan we also remove the following for the same reasons?
mm/userfaultfd.c +0
-0mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 265
Quoted 5 lines
mm/userfaultfd.c +0
-1@@ -262,7 +262,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,pte_t dst_pteval;BUG_ON (dst_addr >= dst_start + len); -VM_BUG_ON (dst_addr & ~huge_page_mask (h));Thanks for your comment.
Quoted 6 lines
Quoted 2 lines
It looks good, while I lack some knowledge between
vma_hpagesizeandhuge_page_mask().
vma_hpagesizeis just a local variable used so that repeated calls tovma_kernel_pagesize()orhuge_page_size()are not necessary.
Thanks for your confirmation. If this is the case, we can remove this BUG_ON safely.
Quoted 9 lines
Quoted 2 lines
If they are the same, why not use the same interface for all those checks in this function?
If we remove the
VM_BUG_ON, that is the only use ofhuge_page_mask()in the function.We can can also eliminate a call to
huge_page_size()by making this change.
Review static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 278
Quoted 8 lines
(quoted diff) +1
-1@@ -273,7 +272,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, mutex_lock(&hugetlb_fault_mutex_table[hash]); err = -ENOMEM; - dst_pte = huge_pte_alloc(dst_mm, dst_addr, huge_page_size(h)); + dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize); if (!dst_pte) { mutex_unlock(&hugetlb_fault_mutex_table[hash]); goto out_unlock;
Agree, and also with this I think
mm/userfaultfd.c +1 -1
@@ -315,7 +315,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, err =copy_huge_page_from_user (page, (const void __user *)src_addr, -pages_per_huge_page (h), true); + vma_hpagesize /PAGE_SIZE , true);if (unlikely (err)) { err = -EFAULT ; goto out;
After these cleanup, we use vma_pagesize to deal with all page size related calculation in this function, which looks more consistent to me.
Does it looks good to you?
Quoted 2 lines
Signature · 1 line
Mike Kravetz
Signature · 2 lines
Wei Yang Help you, Help me
Quoted 72 lines · Wei Yang
Quoted 27 lines · Mike Kravetz
Quoted 26 lines · Wei Yang
Quoted 17 lines · Mike Kravetz
Quoted 8 lines · Wei Yang
The warning here is to make sure
address(dst_addr)and length(len - copied) are huge page size aligned.While this is ensured by:
dst_startand len is huge page size aligned
dst_addrequals todst_startand increase huge page size each time copied increase huge page size each timeCan we also remove the following for the same reasons?
mm/userfaultfd.c +0
-0mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 265
Quoted 5 lines
mm/userfaultfd.c +0
-1@@ -262,7 +262,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,pte_t dst_pteval;BUG_ON (dst_addr >= dst_start + len); -VM_BUG_ON (dst_addr & ~huge_page_mask (h));Thanks for your comment.
Quoted 6 lines
Quoted 2 lines
It looks good, while I lack some knowledge between
vma_hpagesizeandhuge_page_mask().
vma_hpagesizeis just a local variable used so that repeated calls tovma_kernel_pagesize()orhuge_page_size()are not necessary.Thanks for your confirmation. If this is the case, we can remove this
BUG_ONsafely.Quoted 9 lines
Quoted 2 lines
If they are the same, why not use the same interface for all those checks in this function?
If we remove the
VM_BUG_ON, that is the only use ofhuge_page_mask()in the function.We can can also eliminate a call to
huge_page_size()by making this change.Review static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, · line 278
Quoted 8 lines
(quoted diff) +1
-1@@ -273,7 +272,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, mutex_lock(&hugetlb_fault_mutex_table[hash]); err = -ENOMEM; - dst_pte = huge_pte_alloc(dst_mm, dst_addr, huge_page_size(h)); + dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize); if (!dst_pte) { mutex_unlock(&hugetlb_fault_mutex_table[hash]); goto out_unlock;Agree, and also with this I think
mm/userfaultfd.c +1
-1@@ -315,7 +315,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm, err =copy_huge_page_from_user (page, (const void __user *)src_addr, -pages_per_huge_page (h), true); + vma_hpagesize /PAGE_SIZE , true);if (unlikely (err)) { err = -EFAULT ; goto out;
Quoted 4 lines
After these cleanup, we use
vma_pagesizeto deal with all page size related calculation in this function, which looks more consistent to me.Does it looks good to you?
Yes, that looks good.
Thanks for cleaning up this code.
Signature · 1 line
Mike Kravetz