Re: [PATCH 1/2] userfaultfd: remove one unnecessary warn_on in __mcopy_atomic_hugetlb

[Mike Kravetz <mike.kravetz@oracle.com>]

On 9/25/19 7:56 PM, Wei Yang wrote:
> On Wed, Sep 25, 2019 at 07:10:46PM -0700, Mike Kravetz wrote:
>> On 9/25/19 5:35 PM, Wei Yang wrote:
>>> On Wed, Sep 25, 2019 at 10:44:58AM -0700, Mike Kravetz wrote:
>>>> On 9/25/19 5:18 AM, Wei Yang wrote:
>>>>> 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
>>>>
>>>> Can we also remove the following for the same reasons?
>>>>
>>>> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
>>>> index 640ff2bd9a69..f82d5ec698d8 100644
>>>> --- a/mm/userfaultfd.c
>>>> +++ b/mm/userfaultfd.c
>>>> @@ -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().
>>
>> vma_hpagesize is just a local variable used so that repeated calls to
>> vma_kernel_pagesize() or huge_page_size() are not necessary.
>>
> 
> Thanks for your confirmation. If this is the case, we can remove this BUG_ON
> safely.
> 
>>> 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;
> 
> Agree, and also with this I think
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index c153344774c7..74363f0a0dd0 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -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?

Yes, that looks good.

Thanks for cleaning up this code.
-- 
Mike Kravetz