Re: [PATCH v2 05/11] mm/hmm: improve and rename hmm_vma_fault() to hmm_range_fault() v2

[Jerome Glisse]

On Thu, Mar 28, 2019 at 06:43:51AM -0700, Ira Weiny wrote:
> On Mon, Mar 25, 2019 at 10:40:05AM -0400, Jerome Glisse wrote:
> > From: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > 
> > Rename for consistency between code, comments and documentation. Also
> > improves the comments on all the possible returns values. Improve the
> > function by returning the number of populated entries in pfns array.
> > 
> > Changes since v1:
> >     - updated documentation
> >     - reformated some comments
> > 
> > Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > Reviewed-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > ---
> >  Documentation/vm/hmm.rst |  8 +---
> >  include/linux/hmm.h      | 13 +++++-
> >  mm/hmm.c                 | 91 +++++++++++++++++-----------------------
> >  3 files changed, 52 insertions(+), 60 deletions(-)
> > 
> > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
> > index d9b27bdadd1b..61f073215a8d 100644
> > --- a/Documentation/vm/hmm.rst
> > +++ b/Documentation/vm/hmm.rst
> > @@ -190,13 +190,7 @@ When the device driver wants to populate a range of virtual addresses, it can
> >  use either::
> >  
> >    long hmm_range_snapshot(struct hmm_range *range);
> > -  int hmm_vma_fault(struct vm_area_struct *vma,
> > -                    struct hmm_range *range,
> > -                    unsigned long start,
> > -                    unsigned long end,
> > -                    hmm_pfn_t *pfns,
> > -                    bool write,
> > -                    bool block);
> > +  long hmm_range_fault(struct hmm_range *range, bool block);
> >  
> >  The first one (hmm_range_snapshot()) will only fetch present CPU page table
> >  entries and will not trigger a page fault on missing or non-present entries.
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index 32206b0b1bfd..e9afd23c2eac 100644
> > --- a/include/linux/hmm.h
> > +++ b/include/linux/hmm.h
> > @@ -391,7 +391,18 @@ bool hmm_vma_range_done(struct hmm_range *range);
> >   *
> >   * See the function description in mm/hmm.c for further documentation.
> >   */
> > -int hmm_vma_fault(struct hmm_range *range, bool block);
> > +long hmm_range_fault(struct hmm_range *range, bool block);
> > +
> > +/* This is a temporary helper to avoid merge conflict between trees. */
> > +static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> > +{
> > +	long ret = hmm_range_fault(range, block);
> > +	if (ret == -EBUSY)
> > +		ret = -EAGAIN;
> > +	else if (ret == -EAGAIN)
> > +		ret = -EBUSY;
> > +	return ret < 0 ? ret : 0;
> > +}
> >  
> >  /* Below are for HMM internal use only! Not to be used by device driver! */
> >  void hmm_mm_destroy(struct mm_struct *mm);
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 91361aa74b8b..7860e63c3ba7 100644
> > --- a/mm/hmm.c
> > +++ b/mm/hmm.c
> > @@ -336,13 +336,13 @@ static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr,
> >  	flags |= write_fault ? FAULT_FLAG_WRITE : 0;
> >  	ret = handle_mm_fault(vma, addr, flags);
> >  	if (ret & VM_FAULT_RETRY)
> > -		return -EBUSY;
> > +		return -EAGAIN;
> >  	if (ret & VM_FAULT_ERROR) {
> >  		*pfn = range->values[HMM_PFN_ERROR];
> >  		return -EFAULT;
> >  	}
> >  
> > -	return -EAGAIN;
> > +	return -EBUSY;
> >  }
> >  
> >  static int hmm_pfns_bad(unsigned long addr,
> > @@ -368,7 +368,7 @@ static int hmm_pfns_bad(unsigned long addr,
> >   * @fault: should we fault or not ?
> >   * @write_fault: write fault ?
> >   * @walk: mm_walk structure
> > - * Returns: 0 on success, -EAGAIN after page fault, or page fault error
> > + * Returns: 0 on success, -EBUSY after page fault, or page fault error
> >   *
> >   * This function will be called whenever pmd_none() or pte_none() returns true,
> >   * or whenever there is no page directory covering the virtual address range.
> > @@ -391,12 +391,12 @@ static int hmm_vma_walk_hole_(unsigned long addr, unsigned long end,
> >  
> >  			ret = hmm_vma_do_fault(walk, addr, write_fault,
> >  					       &pfns[i]);
> > -			if (ret != -EAGAIN)
> > +			if (ret != -EBUSY)
> >  				return ret;
> >  		}
> >  	}
> >  
> > -	return (fault || write_fault) ? -EAGAIN : 0;
> > +	return (fault || write_fault) ? -EBUSY : 0;
> >  }
> >  
> >  static inline void hmm_pte_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
> > @@ -527,11 +527,11 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
> >  	uint64_t orig_pfn = *pfn;
> >  
> >  	*pfn = range->values[HMM_PFN_NONE];
> > -	cpu_flags = pte_to_hmm_pfn_flags(range, pte);
> > -	hmm_pte_need_fault(hmm_vma_walk, orig_pfn, cpu_flags,
> > -			   &fault, &write_fault);
> > +	fault = write_fault = false;
> >  
> >  	if (pte_none(pte)) {
> > +		hmm_pte_need_fault(hmm_vma_walk, orig_pfn, 0,
> > +				   &fault, &write_fault);
> 
> This really threw me until I applied the patches to a tree.  It looks like this
> is just optimizing away a pte_none() check.  The only functional change which
> was mentioned was returning the number of populated pfns.  So I spent a bit of
> time trying to figure out why hmm_pte_need_fault() needed to move _here_ to do
> that...  :-(
> 
> It would have been nice to have said something about optimizing in the commit
> message.

Yes i should have added that to the commit message i forgot.

> 
> >  		if (fault || write_fault)
> >  			goto fault;
> >  		return 0;
> > @@ -570,7 +570,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
> >  				hmm_vma_walk->last = addr;
> >  				migration_entry_wait(vma->vm_mm,
> >  						     pmdp, addr);
> > -				return -EAGAIN;
> > +				return -EBUSY;
> >  			}
> >  			return 0;
> >  		}
> > @@ -578,6 +578,10 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
> >  		/* Report error for everything else */
> >  		*pfn = range->values[HMM_PFN_ERROR];
> >  		return -EFAULT;
> > +	} else {
> > +		cpu_flags = pte_to_hmm_pfn_flags(range, pte);
> > +		hmm_pte_need_fault(hmm_vma_walk, orig_pfn, cpu_flags,
> > +				   &fault, &write_fault);
> 
> Looks like the same situation as above.
> 
> >  	}
> >  
> >  	if (fault || write_fault)
> > @@ -628,7 +632,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
> >  		if (fault || write_fault) {
> >  			hmm_vma_walk->last = addr;
> >  			pmd_migration_entry_wait(vma->vm_mm, pmdp);
> > -			return -EAGAIN;
> > +			return -EBUSY;
> 
> While I am at it.  Why are we swapping EAGAIN and EBUSY everywhere?

It is a part of the API change when going from hmm_vma_fault() to
hmm_range_fault() and unifying the return values with the old
hmm_vma_get_pfns() so that all API have the same meaning behind
the same return value.

Cheers,
Jérôme