Re: [PATCH 5/5] mm: merge hmm_vma_do_fault into into hmm_vma_walk_hole_

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Mar 16, 2020 at 02:53:10PM +0100, Christoph Hellwig wrote:
> There is no good reason for this split, as it just obsfucates the flow.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>  mm/hmm.c | 49 ++++++++++++++++---------------------------------
>  1 file changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 707edba850de..180e398170b0 100644
> +++ b/mm/hmm.c
> @@ -33,32 +33,6 @@ struct hmm_vma_walk {
>  	unsigned int		flags;
>  };
>  
> -static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr,
> -			    bool write_fault, uint64_t *pfn)
> -{
> -	unsigned int flags = FAULT_FLAG_REMOTE;
> -	struct hmm_vma_walk *hmm_vma_walk = walk->private;
> -	struct hmm_range *range = hmm_vma_walk->range;
> -	struct vm_area_struct *vma = walk->vma;
> -	vm_fault_t ret;
> -
> -	if (!vma)
> -		goto err;
> -
> -	if (write_fault)
> -		flags |= FAULT_FLAG_WRITE;
> -
> -	ret = handle_mm_fault(vma, addr, flags);
> -	if (ret & VM_FAULT_ERROR)
> -		goto err;
> -
> -	return -EBUSY;
> -
> -err:
> -	*pfn = range->values[HMM_PFN_ERROR];
> -	return -EFAULT;
> -}
> -
>  static int hmm_pfns_fill(unsigned long addr, unsigned long end,
>  		struct hmm_range *range, enum hmm_pfn_value_e value)
>  {
> @@ -90,25 +64,34 @@ static int hmm_vma_fault(unsigned long addr, unsigned long end,
>  {
>  	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>  	struct hmm_range *range = hmm_vma_walk->range;
> +	struct vm_area_struct *vma = walk->vma;
>  	uint64_t *pfns = range->pfns;
>  	unsigned long i = (addr - range->start) >> PAGE_SHIFT;
> +	unsigned int fault_flags = FAULT_FLAG_REMOTE;
>  
>  	WARN_ON_ONCE(!fault && !write_fault);
>  	hmm_vma_walk->last = addr;
>  
> -	if (write_fault && walk->vma && !(walk->vma->vm_flags & VM_WRITE))
> -		return -EPERM;
> +	if (!vma)
> +		goto out_error;
> +
> +	if (write_fault) {
> +		if (!(vma->vm_flags & VM_WRITE))
> +			return -EPERM;
> +		fault_flags |= FAULT_FLAG_WRITE;
> +	}
>  
>  	for (; addr < end; addr += PAGE_SIZE, i++) {
> -		int ret;
> -
> +		if (handle_mm_fault(vma, addr, fault_flags) & VM_FAULT_ERROR)
> +			goto out_error;
>  		pfns[i] = range->values[HMM_PFN_NONE];
> -		ret = hmm_vma_do_fault(walk, addr, write_fault, &pfns[i]);
> -		if (ret != -EBUSY)
> -			return ret;
>  	}

Yes, this is much better

>  	return -EBUSY;
> +
> +out_error:
> +	pfns[i] = range->values[HMM_PFN_ERROR];
> +	return -EFAULT;

I've also got a patch deleting these confusing HMM_PFN_ERRORs. They
are not applied consistently and no caller would scan the output for
ERROR on some failures. For instance the above doesn't set it on EPERM.

So this can just be 'return -EFAULT' instead of 'goto out_error'

I can fold that in if you agree:

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason