Re: [PATCH 15/33] userfaultfd: hugetlbfs: add __mcopy_atomic_hugetlb for huge page UFFDIO_COPY

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

On 11/03/2016 03:15 AM, Hillf Danton wrote:
> [out of topic] Cc list is edited to quite mail agent warning:  
> -"Dr. David Alan Gilbert"@v2.random; " <dgilbert@redhat.com> 
> +"Dr. David Alan Gilbert" <dgilbert@redhat.com>
> -Pavel Emelyanov <xemul@parallels.com>"@v2.random
> +Pavel Emelyanov <xemul@parallels.com>
> -Michael Rapoport <RAPOPORT@il.ibm.com>
> +Mike Rapoport <rppt@linux.vnet.ibm.com>
> 
> 
> On Thursday, November 03, 2016 3:34 AM Andrea Arcangeli wrote: 
>> +
>> +#ifdef CONFIG_HUGETLB_PAGE
>> +/*
>> + * __mcopy_atomic processing for HUGETLB vmas.  Note that this routine is
>> + * called with mmap_sem held, it will release mmap_sem before returning.
>> + */
>> +static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
>> +					      struct vm_area_struct *dst_vma,
>> +					      unsigned long dst_start,
>> +					      unsigned long src_start,
>> +					      unsigned long len,
>> +					      bool zeropage)
>> +{
>> +	ssize_t err;
>> +	pte_t *dst_pte;
>> +	unsigned long src_addr, dst_addr;
>> +	long copied;
>> +	struct page *page;
>> +	struct hstate *h;
>> +	unsigned long vma_hpagesize;
>> +	pgoff_t idx;
>> +	u32 hash;
>> +	struct address_space *mapping;
>> +
>> +	/*
>> +	 * There is no default zero huge page for all huge page sizes as
>> +	 * supported by hugetlb.  A PMD_SIZE huge pages may exist as used
>> +	 * by THP.  Since we can not reliably insert a zero page, this
>> +	 * feature is not supported.
>> +	 */
>> +	if (zeropage)
>> +		return -EINVAL;
> 
> Release mmap_sem before return?
> 
>> +
>> +	src_addr = src_start;
>> +	dst_addr = dst_start;
>> +	copied = 0;
>> +	page = NULL;
>> +	vma_hpagesize = vma_kernel_pagesize(dst_vma);
>> +
>> +retry:
>> +	/*
>> +	 * On routine entry dst_vma is set.  If we had to drop mmap_sem and
>> +	 * retry, dst_vma will be set to NULL and we must lookup again.
>> +	 */
>> +	err = -EINVAL;
>> +	if (!dst_vma) {
>> +		dst_vma = find_vma(dst_mm, dst_start);
> 
> In case of retry, s/dst_start/dst_addr/?
> And check if we find a valid vma?
> 
>> @@ -182,6 +355,13 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
>>  		goto out_unlock;
>>
>>  	/*
>> +	 * If this is a HUGETLB vma, pass off to appropriate routine
>> +	 */
>> +	if (dst_vma->vm_flags & VM_HUGETLB)
>> +		return  __mcopy_atomic_hugetlb(dst_mm, dst_vma, dst_start,
>> +						src_start, len, false);
> 
> Use is_vm_hugetlb_page()? 
> 
> 

Thanks Hillf, all valid points.  I will create another version of
this patch.

-- 
Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>