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
