Re: [PATCH 4/5] mm: don't handle the non-fault case in hmm_vma_walk_hole_

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Mar 16, 2020 at 02:53:09PM +0100, Christoph Hellwig wrote:
> There is just a single caller using hmm_vma_walk_hole_ for the non-fault
> case.  Use hmm_pfns_fill to fill the whole pfn array with zeroes in only
> caller for the non-fault case and remove the non-fault path from
> hmm_vma_walk_hole_.
> 
> Also rename the function to hmm_vma_fault to better describe what it
> does.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/hmm.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)

Ah, I did nearly the same in my the next series, I'm cooking:

https://github.com/jgunthorpe/linux/commit/078e10ca5919f2c263c245784fb5fe63ddbb61f4

But this can go first, I was probably going to break mine up anyhow.

> +               pfns[i] = range->values[HMM_PFN_NONE];

Since the function always returns -EBUSY now, it should not set an
output. This is an existing bug. Since there is no purpose now that
the caller does the fill, this patch should drop the above and have
the fixes line.

>  static inline void hmm_pte_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
> @@ -193,7 +190,10 @@ static int hmm_vma_walk_hole(unsigned long addr, unsigned long end,
>  	pfns = &range->pfns[i];
>  	hmm_range_need_fault(hmm_vma_walk, pfns, npages,
>  			     0, &fault, &write_fault);
> -	return hmm_vma_walk_hole_(addr, end, fault, write_fault, walk);
> +	if (fault || write_fault)
> +		return hmm_vma_fault(addr, end, fault, write_fault, walk);
> +	hmm_vma_walk->last = addr;
> +	return hmm_pfns_fill(addr, end, range, HMM_PFN_NONE);

This is also the only caller of hmm_vma_fault() that could have
walk->vma == NULL, and NULL vma + a fault == -EFAULT

Jason