Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 28 Mar 2019 06:11:54 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     jglisse@redhat.com
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        John Hubbard <jhubbard@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v2 06/11] mm/hmm: improve driver API to work and wait
 over a range v2
Message-ID: <20190328131154.GB31324@iweiny-DESK2.sc.intel.com>
References: <20190325144011.10560-1-jglisse@redhat.com>
 <20190325144011.10560-7-jglisse@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190325144011.10560-7-jglisse@redhat.com>
User-Agent: Mutt/1.11.1 (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 25, 2019 at 10:40:06AM -0400, Jerome Glisse wrote:
> From: J?r?me Glisse <jglisse@redhat.com>
> 
> A common use case for HMM mirror is user trying to mirror a range
> and before they could program the hardware it get invalidated by
> some core mm event. Instead of having user re-try right away to
> mirror the range provide a completion mechanism for them to wait
> for any active invalidation affecting the range.
> 
> This also changes how hmm_range_snapshot() and hmm_range_fault()
> works by not relying on vma so that we can drop the mmap_sem
> when waiting and lookup the vma again on retry.
> 
> Changes since v1:
>     - squashed: Dan Carpenter: potential deadlock in nonblocking code
> 
> Signed-off-by: J?r?me Glisse <jglisse@redhat.com>
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>  include/linux/hmm.h | 208 ++++++++++++++---
>  mm/hmm.c            | 528 +++++++++++++++++++++-----------------------
>  2 files changed, 428 insertions(+), 308 deletions(-)
> 
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index e9afd23c2eac..79671036cb5f 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -77,8 +77,34 @@
>  #include <linux/migrate.h>
>  #include <linux/memremap.h>
>  #include <linux/completion.h>
> +#include <linux/mmu_notifier.h>
>  
> -struct hmm;
> +
> +/*
> + * struct hmm - HMM per mm struct
> + *
> + * @mm: mm struct this HMM struct is bound to
> + * @lock: lock protecting ranges list
> + * @ranges: list of range being snapshotted
> + * @mirrors: list of mirrors for this mm
> + * @mmu_notifier: mmu notifier to track updates to CPU page table
> + * @mirrors_sem: read/write semaphore protecting the mirrors list
> + * @wq: wait queue for user waiting on a range invalidation
> + * @notifiers: count of active mmu notifiers
> + * @dead: is the mm dead ?
> + */
> +struct hmm {
> +	struct mm_struct	*mm;
> +	struct kref		kref;
> +	struct mutex		lock;
> +	struct list_head	ranges;
> +	struct list_head	mirrors;
> +	struct mmu_notifier	mmu_notifier;
> +	struct rw_semaphore	mirrors_sem;
> +	wait_queue_head_t	wq;
> +	long			notifiers;
> +	bool			dead;
> +};
>  
>  /*
>   * hmm_pfn_flag_e - HMM flag enums
> @@ -155,6 +181,38 @@ struct hmm_range {
>  	bool			valid;
>  };
>  
> +/*
> + * hmm_range_wait_until_valid() - wait for range to be valid
> + * @range: range affected by invalidation to wait on
> + * @timeout: time out for wait in ms (ie abort wait after that period of time)
> + * Returns: true if the range is valid, false otherwise.
> + */
> +static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> +					      unsigned long timeout)
> +{
> +	/* Check if mm is dead ? */
> +	if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> +		range->valid = false;
> +		return false;
> +	}
> +	if (range->valid)
> +		return true;
> +	wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> +			   msecs_to_jiffies(timeout));
> +	/* Return current valid status just in case we get lucky */
> +	return range->valid;
> +}
> +
> +/*
> + * hmm_range_valid() - test if a range is valid or not
> + * @range: range
> + * Returns: true if the range is valid, false otherwise.
> + */
> +static inline bool hmm_range_valid(struct hmm_range *range)
> +{
> +	return range->valid;
> +}
> +
>  /*
>   * hmm_pfn_to_page() - return struct page pointed to by a valid HMM pfn
>   * @range: range use to decode HMM pfn value
> @@ -357,51 +415,133 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror);
>  
>  
>  /*
> - * To snapshot the CPU page table, call hmm_vma_get_pfns(), then take a device
> - * driver lock that serializes device page table updates, then call
> - * hmm_vma_range_done(), to check if the snapshot is still valid. The same
> - * device driver page table update lock must also be used in the
> - * hmm_mirror_ops.sync_cpu_device_pagetables() callback, so that CPU page
> - * table invalidation serializes on it.
> + * To snapshot the CPU page table you first have to call hmm_range_register()
> + * to register the range. If hmm_range_register() return an error then some-
> + * thing is horribly wrong and you should fail loudly. If it returned true then
> + * you can wait for the range to be stable with hmm_range_wait_until_valid()
> + * function, a range is valid when there are no concurrent changes to the CPU
> + * page table for the range.
> + *
> + * Once the range is valid you can call hmm_range_snapshot() if that returns
> + * without error then you can take your device page table lock (the same lock
> + * you use in the HMM mirror sync_cpu_device_pagetables() callback). After
> + * taking that lock you have to check the range validity, if it is still valid
> + * (ie hmm_range_valid() returns true) then you can program the device page
> + * table, otherwise you have to start again. Pseudo code:
> + *
> + *      mydevice_prefault(mydevice, mm, start, end)
> + *      {
> + *          struct hmm_range range;
> + *          ...
>   *
> - * YOU MUST CALL hmm_vma_range_done() ONCE AND ONLY ONCE EACH TIME YOU CALL
> - * hmm_range_snapshot() WITHOUT ERROR !
> + *          ret = hmm_range_register(&range, mm, start, end);
> + *          if (ret)
> + *              return ret;
>   *
> - * IF YOU DO NOT FOLLOW THE ABOVE RULE THE SNAPSHOT CONTENT MIGHT BE INVALID !
> - */
> -long hmm_range_snapshot(struct hmm_range *range);
> -bool hmm_vma_range_done(struct hmm_range *range);
> -
> -
> -/*
> - * Fault memory on behalf of device driver. Unlike handle_mm_fault(), this will
> - * not migrate any device memory back to system memory. The HMM pfn array will
> - * be updated with the fault result and current snapshot of the CPU page table
> - * for the range.
> + *          down_read(mm->mmap_sem);
> + *      again:
> + *
> + *          if (!hmm_range_wait_until_valid(&range, TIMEOUT)) {
> + *              up_read(&mm->mmap_sem);
> + *              hmm_range_unregister(range);
> + *              // Handle time out, either sleep or retry or something else
> + *              ...
> + *              return -ESOMETHING; || goto again;
> + *          }
> + *
> + *          ret = hmm_range_snapshot(&range); or hmm_range_fault(&range);
> + *          if (ret == -EAGAIN) {
> + *              down_read(mm->mmap_sem);
> + *              goto again;
> + *          } else if (ret == -EBUSY) {
> + *              goto again;
> + *          }
> + *
> + *          up_read(&mm->mmap_sem);
> + *          if (ret) {
> + *              hmm_range_unregister(range);
> + *              return ret;
> + *          }
> + *
> + *          // It might not have snap-shoted the whole range but only the first
> + *          // npages, the return values is the number of valid pages from the
> + *          // start of the range.
> + *          npages = ret;
>   *
> - * The mmap_sem must be taken in read mode before entering and it might be
> - * dropped by the function if the block argument is false. In that case, the
> - * function returns -EAGAIN.
> + *          ...
>   *
> - * Return value does not reflect if the fault was successful for every single
> - * address or not. Therefore, the caller must to inspect the HMM pfn array to
> - * determine fault status for each address.
> + *          mydevice_page_table_lock(mydevice);
> + *          if (!hmm_range_valid(range)) {
> + *              mydevice_page_table_unlock(mydevice);
> + *              goto again;
> + *          }
>   *
> - * Trying to fault inside an invalid vma will result in -EINVAL.
> + *          mydevice_populate_page_table(mydevice, range, npages);
> + *          ...
> + *          mydevice_take_page_table_unlock(mydevice);
> + *          hmm_range_unregister(range);
>   *
> - * See the function description in mm/hmm.c for further documentation.
> + *          return 0;
> + *      }
> + *
> + * The same scheme apply to hmm_range_fault() (ie replace hmm_range_snapshot()
> + * with hmm_range_fault() in above pseudo code).
> + *
> + * YOU MUST CALL hmm_range_unregister() ONCE AND ONLY ONCE EACH TIME YOU CALL
> + * hmm_range_register() AND hmm_range_register() RETURNED TRUE ! IF YOU DO NOT
> + * FOLLOW THIS RULE MEMORY CORRUPTION WILL ENSUE !
>   */
> +int hmm_range_register(struct hmm_range *range,
> +		       struct mm_struct *mm,
> +		       unsigned long start,
> +		       unsigned long end);
> +void hmm_range_unregister(struct hmm_range *range);
> +long hmm_range_snapshot(struct hmm_range *range);
>  long hmm_range_fault(struct hmm_range *range, bool block);
>  
> +/*
> + * HMM_RANGE_DEFAULT_TIMEOUT - default timeout (ms) when waiting for a range
> + *
> + * When waiting for mmu notifiers we need some kind of time out otherwise we
> + * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to
> + * wait already.
> + */
> +#define HMM_RANGE_DEFAULT_TIMEOUT 1000
> +
>  /* This is a temporary helper to avoid merge conflict between trees. */
> +static inline bool hmm_vma_range_done(struct hmm_range *range)
> +{
> +	bool ret = hmm_range_valid(range);
> +
> +	hmm_range_unregister(range);
> +	return ret;
> +}
> +
>  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;
> +	long ret;
> +
> +	ret = hmm_range_register(range, range->vma->vm_mm,
> +				 range->start, range->end);
> +	if (ret)
> +		return (int)ret;
> +
> +	if (!hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT)) {
> +		up_read(&range->vma->vm_mm->mmap_sem);

Where is the down_read() which correspond to this?

> +		return -EAGAIN;
> +	}
> +
> +	ret = hmm_range_fault(range, block);
> +	if (ret <= 0) {
> +		if (ret == -EBUSY || !ret) {
> +			up_read(&range->vma->vm_mm->mmap_sem);

Or this...?

> +			ret = -EBUSY;
> +		} else if (ret == -EAGAIN)
> +			ret = -EBUSY;
> +		hmm_range_unregister(range);
> +		return ret;
> +	}

And is the side effect of this call that the mmap_sem has been taken?
Or is the side effect that the mmap_sem was released?

I'm not saying this is wrong, but I can't tell so it seems like a comment on
the function would help.

Ira

> +	return 0;
>  }
>  
>  /* Below are for HMM internal use only! Not to be used by device driver! */
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 7860e63c3ba7..fa9498eeb9b6 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -38,26 +38,6 @@
>  #if IS_ENABLED(CONFIG_HMM_MIRROR)
>  static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
>  
> -/*
> - * struct hmm - HMM per mm struct
> - *
> - * @mm: mm struct this HMM struct is bound to
> - * @lock: lock protecting ranges list
> - * @ranges: list of range being snapshotted
> - * @mirrors: list of mirrors for this mm
> - * @mmu_notifier: mmu notifier to track updates to CPU page table
> - * @mirrors_sem: read/write semaphore protecting the mirrors list
> - */
> -struct hmm {
> -	struct mm_struct	*mm;
> -	struct kref		kref;
> -	spinlock_t		lock;
> -	struct list_head	ranges;
> -	struct list_head	mirrors;
> -	struct mmu_notifier	mmu_notifier;
> -	struct rw_semaphore	mirrors_sem;
> -};
> -
>  static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
>  {
>  	struct hmm *hmm = READ_ONCE(mm->hmm);
> @@ -87,12 +67,15 @@ static struct hmm *hmm_register(struct mm_struct *mm)
>  	hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
>  	if (!hmm)
>  		return NULL;
> +	init_waitqueue_head(&hmm->wq);
>  	INIT_LIST_HEAD(&hmm->mirrors);
>  	init_rwsem(&hmm->mirrors_sem);
>  	hmm->mmu_notifier.ops = NULL;
>  	INIT_LIST_HEAD(&hmm->ranges);
> -	spin_lock_init(&hmm->lock);
> +	mutex_init(&hmm->lock);
>  	kref_init(&hmm->kref);
> +	hmm->notifiers = 0;
> +	hmm->dead = false;
>  	hmm->mm = mm;
>  
>  	spin_lock(&mm->page_table_lock);
> @@ -154,6 +137,7 @@ void hmm_mm_destroy(struct mm_struct *mm)
>  	mm->hmm = NULL;
>  	if (hmm) {
>  		hmm->mm = NULL;
> +		hmm->dead = true;
>  		spin_unlock(&mm->page_table_lock);
>  		hmm_put(hmm);
>  		return;
> @@ -162,43 +146,22 @@ void hmm_mm_destroy(struct mm_struct *mm)
>  	spin_unlock(&mm->page_table_lock);
>  }
>  
> -static int hmm_invalidate_range(struct hmm *hmm, bool device,
> -				const struct hmm_update *update)
> +static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>  {
> +	struct hmm *hmm = mm_get_hmm(mm);
>  	struct hmm_mirror *mirror;
>  	struct hmm_range *range;
>  
> -	spin_lock(&hmm->lock);
> -	list_for_each_entry(range, &hmm->ranges, list) {
> -		if (update->end < range->start || update->start >= range->end)
> -			continue;
> +	/* Report this HMM as dying. */
> +	hmm->dead = true;
>  
> +	/* Wake-up everyone waiting on any range. */
> +	mutex_lock(&hmm->lock);
> +	list_for_each_entry(range, &hmm->ranges, list) {
>  		range->valid = false;
>  	}
> -	spin_unlock(&hmm->lock);
> -
> -	if (!device)
> -		return 0;
> -
> -	down_read(&hmm->mirrors_sem);
> -	list_for_each_entry(mirror, &hmm->mirrors, list) {
> -		int ret;
> -
> -		ret = mirror->ops->sync_cpu_device_pagetables(mirror, update);
> -		if (!update->blockable && ret == -EAGAIN) {
> -			up_read(&hmm->mirrors_sem);
> -			return -EAGAIN;
> -		}
> -	}
> -	up_read(&hmm->mirrors_sem);
> -
> -	return 0;
> -}
> -
> -static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> -{
> -	struct hmm_mirror *mirror;
> -	struct hmm *hmm = mm_get_hmm(mm);
> +	wake_up_all(&hmm->wq);
> +	mutex_unlock(&hmm->lock);
>  
>  	down_write(&hmm->mirrors_sem);
>  	mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
> @@ -224,36 +187,80 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>  }
>  
>  static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> -			const struct mmu_notifier_range *range)
> +			const struct mmu_notifier_range *nrange)
>  {
> -	struct hmm *hmm = mm_get_hmm(range->mm);
> +	struct hmm *hmm = mm_get_hmm(nrange->mm);
> +	struct hmm_mirror *mirror;
>  	struct hmm_update update;
> -	int ret;
> +	struct hmm_range *range;
> +	int ret = 0;
>  
>  	VM_BUG_ON(!hmm);
>  
> -	update.start = range->start;
> -	update.end = range->end;
> +	update.start = nrange->start;
> +	update.end = nrange->end;
>  	update.event = HMM_UPDATE_INVALIDATE;
> -	update.blockable = range->blockable;
> -	ret = hmm_invalidate_range(hmm, true, &update);
> +	update.blockable = nrange->blockable;
> +
> +	if (nrange->blockable)
> +		mutex_lock(&hmm->lock);
> +	else if (!mutex_trylock(&hmm->lock)) {
> +		ret = -EAGAIN;
> +		goto out;
> +	}
> +	hmm->notifiers++;
> +	list_for_each_entry(range, &hmm->ranges, list) {
> +		if (update.end < range->start || update.start >= range->end)
> +			continue;
> +
> +		range->valid = false;
> +	}
> +	mutex_unlock(&hmm->lock);
> +
> +	if (nrange->blockable)
> +		down_read(&hmm->mirrors_sem);
> +	else if (!down_read_trylock(&hmm->mirrors_sem)) {
> +		ret = -EAGAIN;
> +		goto out;
> +	}
> +	list_for_each_entry(mirror, &hmm->mirrors, list) {
> +		int ret;
> +
> +		ret = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
> +		if (!update.blockable && ret == -EAGAIN) {
> +			up_read(&hmm->mirrors_sem);
> +			ret = -EAGAIN;
> +			goto out;
> +		}
> +	}
> +	up_read(&hmm->mirrors_sem);
> +
> +out:
>  	hmm_put(hmm);
>  	return ret;
>  }
>  
>  static void hmm_invalidate_range_end(struct mmu_notifier *mn,
> -			const struct mmu_notifier_range *range)
> +			const struct mmu_notifier_range *nrange)
>  {
> -	struct hmm *hmm = mm_get_hmm(range->mm);
> -	struct hmm_update update;
> +	struct hmm *hmm = mm_get_hmm(nrange->mm);
>  
>  	VM_BUG_ON(!hmm);
>  
> -	update.start = range->start;
> -	update.end = range->end;
> -	update.event = HMM_UPDATE_INVALIDATE;
> -	update.blockable = true;
> -	hmm_invalidate_range(hmm, false, &update);
> +	mutex_lock(&hmm->lock);
> +	hmm->notifiers--;
> +	if (!hmm->notifiers) {
> +		struct hmm_range *range;
> +
> +		list_for_each_entry(range, &hmm->ranges, list) {
> +			if (range->valid)
> +				continue;
> +			range->valid = true;
> +		}
> +		wake_up_all(&hmm->wq);
> +	}
> +	mutex_unlock(&hmm->lock);
> +
>  	hmm_put(hmm);
>  }
>  
> @@ -405,7 +412,6 @@ static inline void hmm_pte_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
>  {
>  	struct hmm_range *range = hmm_vma_walk->range;
>  
> -	*fault = *write_fault = false;
>  	if (!hmm_vma_walk->fault)
>  		return;
>  
> @@ -444,10 +450,11 @@ static void hmm_range_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
>  		return;
>  	}
>  
> +	*fault = *write_fault = false;
>  	for (i = 0; i < npages; ++i) {
>  		hmm_pte_need_fault(hmm_vma_walk, pfns[i], cpu_flags,
>  				   fault, write_fault);
> -		if ((*fault) || (*write_fault))
> +		if ((*write_fault))
>  			return;
>  	}
>  }
> @@ -702,162 +709,152 @@ static void hmm_pfns_special(struct hmm_range *range)
>  }
>  
>  /*
> - * hmm_range_snapshot() - snapshot CPU page table for a range
> + * hmm_range_register() - start tracking change to CPU page table over a range
>   * @range: range
> - * Returns: number of valid pages in range->pfns[] (from range start
> - *          address). This may be zero. If the return value is negative,
> - *          then one of the following values may be returned:
> + * @mm: the mm struct for the range of virtual address
> + * @start: start virtual address (inclusive)
> + * @end: end virtual address (exclusive)
> + * Returns 0 on success, -EFAULT if the address space is no longer valid
>   *
> - *           -EINVAL  invalid arguments or mm or virtual address are in an
> - *                    invalid vma (ie either hugetlbfs or device file vma).
> - *           -EPERM   For example, asking for write, when the range is
> - *                    read-only
> - *           -EAGAIN  Caller needs to retry
> - *           -EFAULT  Either no valid vma exists for this range, or it is
> - *                    illegal to access the range
> - *
> - * This snapshots the CPU page table for a range of virtual addresses. Snapshot
> - * validity is tracked by range struct. See hmm_vma_range_done() for further
> - * information.
> + * Track updates to the CPU page table see include/linux/hmm.h
>   */
> -long hmm_range_snapshot(struct hmm_range *range)
> +int hmm_range_register(struct hmm_range *range,
> +		       struct mm_struct *mm,
> +		       unsigned long start,
> +		       unsigned long end)
>  {
> -	struct vm_area_struct *vma = range->vma;
> -	struct hmm_vma_walk hmm_vma_walk;
> -	struct mm_walk mm_walk;
> -	struct hmm *hmm;
> -
> +	range->start = start & PAGE_MASK;
> +	range->end = end & PAGE_MASK;
> +	range->valid = false;
>  	range->hmm = NULL;
>  
> -	/* Sanity check, this really should not happen ! */
> -	if (range->start < vma->vm_start || range->start >= vma->vm_end)
> -		return -EINVAL;
> -	if (range->end < vma->vm_start || range->end > vma->vm_end)
> +	if (range->start >= range->end)
>  		return -EINVAL;
>  
> -	hmm = hmm_register(vma->vm_mm);
> -	if (!hmm)
> -		return -ENOMEM;
> +	range->hmm = hmm_register(mm);
> +	if (!range->hmm)
> +		return -EFAULT;
>  
>  	/* Check if hmm_mm_destroy() was call. */
> -	if (hmm->mm == NULL) {
> -		hmm_put(hmm);
> -		return -EINVAL;
> +	if (range->hmm->mm == NULL || range->hmm->dead) {
> +		hmm_put(range->hmm);
> +		return -EFAULT;
>  	}
>  
> -	/* FIXME support hugetlb fs */
> -	if (is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL) ||
> -			vma_is_dax(vma)) {
> -		hmm_pfns_special(range);
> -		hmm_put(hmm);
> -		return -EINVAL;
> -	}
> +	/* Initialize range to track CPU page table update */
> +	mutex_lock(&range->hmm->lock);
>  
> -	if (!(vma->vm_flags & VM_READ)) {
> -		/*
> -		 * If vma do not allow read access, then assume that it does
> -		 * not allow write access, either. Architecture that allow
> -		 * write without read access are not supported by HMM, because
> -		 * operations such has atomic access would not work.
> -		 */
> -		hmm_pfns_clear(range, range->pfns, range->start, range->end);
> -		hmm_put(hmm);
> -		return -EPERM;
> -	}
> +	list_add_rcu(&range->list, &range->hmm->ranges);
>  
> -	/* Initialize range to track CPU page table update */
> -	spin_lock(&hmm->lock);
> -	range->valid = true;
> -	list_add_rcu(&range->list, &hmm->ranges);
> -	spin_unlock(&hmm->lock);
> -
> -	hmm_vma_walk.fault = false;
> -	hmm_vma_walk.range = range;
> -	mm_walk.private = &hmm_vma_walk;
> -	hmm_vma_walk.last = range->start;
> -
> -	mm_walk.vma = vma;
> -	mm_walk.mm = vma->vm_mm;
> -	mm_walk.pte_entry = NULL;
> -	mm_walk.test_walk = NULL;
> -	mm_walk.hugetlb_entry = NULL;
> -	mm_walk.pmd_entry = hmm_vma_walk_pmd;
> -	mm_walk.pte_hole = hmm_vma_walk_hole;
> -
> -	walk_page_range(range->start, range->end, &mm_walk);
>  	/*
> -	 * Transfer hmm reference to the range struct it will be drop inside
> -	 * the hmm_vma_range_done() function (which _must_ be call if this
> -	 * function return 0).
> +	 * If there are any concurrent notifiers we have to wait for them for
> +	 * the range to be valid (see hmm_range_wait_until_valid()).
>  	 */
> -	range->hmm = hmm;
> -	return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
> +	if (!range->hmm->notifiers)
> +		range->valid = true;
> +	mutex_unlock(&range->hmm->lock);
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(hmm_range_snapshot);
> +EXPORT_SYMBOL(hmm_range_register);
>  
>  /*
> - * hmm_vma_range_done() - stop tracking change to CPU page table over a range
> - * @range: range being tracked
> - * Returns: false if range data has been invalidated, true otherwise
> + * hmm_range_unregister() - stop tracking change to CPU page table over a range
> + * @range: range
>   *
>   * Range struct is used to track updates to the CPU page table after a call to
> - * either hmm_vma_get_pfns() or hmm_vma_fault(). Once the device driver is done
> - * using the data,  or wants to lock updates to the data it got from those
> - * functions, it must call the hmm_vma_range_done() function, which will then
> - * stop tracking CPU page table updates.
> - *
> - * Note that device driver must still implement general CPU page table update
> - * tracking either by using hmm_mirror (see hmm_mirror_register()) or by using
> - * the mmu_notifier API directly.
> - *
> - * CPU page table update tracking done through hmm_range is only temporary and
> - * to be used while trying to duplicate CPU page table contents for a range of
> - * virtual addresses.
> - *
> - * There are two ways to use this :
> - * again:
> - *   hmm_vma_get_pfns(range); or hmm_vma_fault(...);
> - *   trans = device_build_page_table_update_transaction(pfns);
> - *   device_page_table_lock();
> - *   if (!hmm_vma_range_done(range)) {
> - *     device_page_table_unlock();
> - *     goto again;
> - *   }
> - *   device_commit_transaction(trans);
> - *   device_page_table_unlock();
> - *
> - * Or:
> - *   hmm_vma_get_pfns(range); or hmm_vma_fault(...);
> - *   device_page_table_lock();
> - *   hmm_vma_range_done(range);
> - *   device_update_page_table(range->pfns);
> - *   device_page_table_unlock();
> + * hmm_range_register(). See include/linux/hmm.h for how to use it.
>   */
> -bool hmm_vma_range_done(struct hmm_range *range)
> +void hmm_range_unregister(struct hmm_range *range)
>  {
> -	bool ret = false;
> -
>  	/* Sanity check this really should not happen. */
> -	if (range->hmm == NULL || range->end <= range->start) {
> -		BUG();
> -		return false;
> -	}
> +	if (range->hmm == NULL || range->end <= range->start)
> +		return;
>  
> -	spin_lock(&range->hmm->lock);
> +	mutex_lock(&range->hmm->lock);
>  	list_del_rcu(&range->list);
> -	ret = range->valid;
> -	spin_unlock(&range->hmm->lock);
> -
> -	/* Is the mm still alive ? */
> -	if (range->hmm->mm == NULL)
> -		ret = false;
> +	mutex_unlock(&range->hmm->lock);
>  
> -	/* Drop reference taken by hmm_vma_fault() or hmm_vma_get_pfns() */
> +	/* Drop reference taken by hmm_range_register() */
> +	range->valid = false;
>  	hmm_put(range->hmm);
>  	range->hmm = NULL;
> -	return ret;
>  }
> -EXPORT_SYMBOL(hmm_vma_range_done);
> +EXPORT_SYMBOL(hmm_range_unregister);
> +
> +/*
> + * hmm_range_snapshot() - snapshot CPU page table for a range
> + * @range: range
> + * Returns: -EINVAL if invalid argument, -ENOMEM out of memory, -EPERM invalid
> + *          permission (for instance asking for write and range is read only),
> + *          -EAGAIN if you need to retry, -EFAULT invalid (ie either no valid
> + *          vma or it is illegal to access that range), number of valid pages
> + *          in range->pfns[] (from range start address).
> + *
> + * This snapshots the CPU page table for a range of virtual addresses. Snapshot
> + * validity is tracked by range struct. See in include/linux/hmm.h for example
> + * on how to use.
> + */
> +long hmm_range_snapshot(struct hmm_range *range)
> +{
> +	unsigned long start = range->start, end;
> +	struct hmm_vma_walk hmm_vma_walk;
> +	struct hmm *hmm = range->hmm;
> +	struct vm_area_struct *vma;
> +	struct mm_walk mm_walk;
> +
> +	/* Check if hmm_mm_destroy() was call. */
> +	if (hmm->mm == NULL || hmm->dead)
> +		return -EFAULT;
> +
> +	do {
> +		/* If range is no longer valid force retry. */
> +		if (!range->valid)
> +			return -EAGAIN;
> +
> +		vma = find_vma(hmm->mm, start);
> +		if (vma == NULL || (vma->vm_flags & VM_SPECIAL))
> +			return -EFAULT;
> +
> +		/* FIXME support hugetlb fs/dax */
> +		if (is_vm_hugetlb_page(vma) || vma_is_dax(vma)) {
> +			hmm_pfns_special(range);
> +			return -EINVAL;
> +		}
> +
> +		if (!(vma->vm_flags & VM_READ)) {
> +			/*
> +			 * If vma do not allow read access, then assume that it
> +			 * does not allow write access, either. HMM does not
> +			 * support architecture that allow write without read.
> +			 */
> +			hmm_pfns_clear(range, range->pfns,
> +				range->start, range->end);
> +			return -EPERM;
> +		}
> +
> +		range->vma = vma;
> +		hmm_vma_walk.last = start;
> +		hmm_vma_walk.fault = false;
> +		hmm_vma_walk.range = range;
> +		mm_walk.private = &hmm_vma_walk;
> +		end = min(range->end, vma->vm_end);
> +
> +		mm_walk.vma = vma;
> +		mm_walk.mm = vma->vm_mm;
> +		mm_walk.pte_entry = NULL;
> +		mm_walk.test_walk = NULL;
> +		mm_walk.hugetlb_entry = NULL;
> +		mm_walk.pmd_entry = hmm_vma_walk_pmd;
> +		mm_walk.pte_hole = hmm_vma_walk_hole;
> +
> +		walk_page_range(start, end, &mm_walk);
> +		start = end;
> +	} while (start < range->end);
> +
> +	return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
> +}
> +EXPORT_SYMBOL(hmm_range_snapshot);
>  
>  /*
>   * hmm_range_fault() - try to fault some address in a virtual address range
> @@ -889,96 +886,79 @@ EXPORT_SYMBOL(hmm_vma_range_done);
>   */
>  long hmm_range_fault(struct hmm_range *range, bool block)
>  {
> -	struct vm_area_struct *vma = range->vma;
> -	unsigned long start = range->start;
> +	unsigned long start = range->start, end;
>  	struct hmm_vma_walk hmm_vma_walk;
> +	struct hmm *hmm = range->hmm;
> +	struct vm_area_struct *vma;
>  	struct mm_walk mm_walk;
> -	struct hmm *hmm;
>  	int ret;
>  
> -	range->hmm = NULL;
> -
> -	/* Sanity check, this really should not happen ! */
> -	if (range->start < vma->vm_start || range->start >= vma->vm_end)
> -		return -EINVAL;
> -	if (range->end < vma->vm_start || range->end > vma->vm_end)
> -		return -EINVAL;
> +	/* Check if hmm_mm_destroy() was call. */
> +	if (hmm->mm == NULL || hmm->dead)
> +		return -EFAULT;
>  
> -	hmm = hmm_register(vma->vm_mm);
> -	if (!hmm) {
> -		hmm_pfns_clear(range, range->pfns, range->start, range->end);
> -		return -ENOMEM;
> -	}
> +	do {
> +		/* If range is no longer valid force retry. */
> +		if (!range->valid) {
> +			up_read(&hmm->mm->mmap_sem);
> +			return -EAGAIN;
> +		}
>  
> -	/* Check if hmm_mm_destroy() was call. */
> -	if (hmm->mm == NULL) {
> -		hmm_put(hmm);
> -		return -EINVAL;
> -	}
> +		vma = find_vma(hmm->mm, start);
> +		if (vma == NULL || (vma->vm_flags & VM_SPECIAL))
> +			return -EFAULT;
>  
> -	/* FIXME support hugetlb fs */
> -	if (is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL) ||
> -			vma_is_dax(vma)) {
> -		hmm_pfns_special(range);
> -		hmm_put(hmm);
> -		return -EINVAL;
> -	}
> +		/* FIXME support hugetlb fs/dax */
> +		if (is_vm_hugetlb_page(vma) || vma_is_dax(vma)) {
> +			hmm_pfns_special(range);
> +			return -EINVAL;
> +		}
>  
> -	if (!(vma->vm_flags & VM_READ)) {
> -		/*
> -		 * If vma do not allow read access, then assume that it does
> -		 * not allow write access, either. Architecture that allow
> -		 * write without read access are not supported by HMM, because
> -		 * operations such has atomic access would not work.
> -		 */
> -		hmm_pfns_clear(range, range->pfns, range->start, range->end);
> -		hmm_put(hmm);
> -		return -EPERM;
> -	}
> +		if (!(vma->vm_flags & VM_READ)) {
> +			/*
> +			 * If vma do not allow read access, then assume that it
> +			 * does not allow write access, either. HMM does not
> +			 * support architecture that allow write without read.
> +			 */
> +			hmm_pfns_clear(range, range->pfns,
> +				range->start, range->end);
> +			return -EPERM;
> +		}
>  
> -	/* Initialize range to track CPU page table update */
> -	spin_lock(&hmm->lock);
> -	range->valid = true;
> -	list_add_rcu(&range->list, &hmm->ranges);
> -	spin_unlock(&hmm->lock);
> -
> -	hmm_vma_walk.fault = true;
> -	hmm_vma_walk.block = block;
> -	hmm_vma_walk.range = range;
> -	mm_walk.private = &hmm_vma_walk;
> -	hmm_vma_walk.last = range->start;
> -
> -	mm_walk.vma = vma;
> -	mm_walk.mm = vma->vm_mm;
> -	mm_walk.pte_entry = NULL;
> -	mm_walk.test_walk = NULL;
> -	mm_walk.hugetlb_entry = NULL;
> -	mm_walk.pmd_entry = hmm_vma_walk_pmd;
> -	mm_walk.pte_hole = hmm_vma_walk_hole;
> +		range->vma = vma;
> +		hmm_vma_walk.last = start;
> +		hmm_vma_walk.fault = true;
> +		hmm_vma_walk.block = block;
> +		hmm_vma_walk.range = range;
> +		mm_walk.private = &hmm_vma_walk;
> +		end = min(range->end, vma->vm_end);
> +
> +		mm_walk.vma = vma;
> +		mm_walk.mm = vma->vm_mm;
> +		mm_walk.pte_entry = NULL;
> +		mm_walk.test_walk = NULL;
> +		mm_walk.hugetlb_entry = NULL;
> +		mm_walk.pmd_entry = hmm_vma_walk_pmd;
> +		mm_walk.pte_hole = hmm_vma_walk_hole;
> +
> +		do {
> +			ret = walk_page_range(start, end, &mm_walk);
> +			start = hmm_vma_walk.last;
> +
> +			/* Keep trying while the range is valid. */
> +		} while (ret == -EBUSY && range->valid);
> +
> +		if (ret) {
> +			unsigned long i;
> +
> +			i = (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
> +			hmm_pfns_clear(range, &range->pfns[i],
> +				hmm_vma_walk.last, range->end);
> +			return ret;
> +		}
> +		start = end;
>  
> -	do {
> -		ret = walk_page_range(start, range->end, &mm_walk);
> -		start = hmm_vma_walk.last;
> -		/* Keep trying while the range is valid. */
> -	} while (ret == -EBUSY && range->valid);
> -
> -	if (ret) {
> -		unsigned long i;
> -
> -		i = (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
> -		hmm_pfns_clear(range, &range->pfns[i], hmm_vma_walk.last,
> -			       range->end);
> -		hmm_vma_range_done(range);
> -		hmm_put(hmm);
> -		return ret;
> -	} else {
> -		/*
> -		 * Transfer hmm reference to the range struct it will be drop
> -		 * inside the hmm_vma_range_done() function (which _must_ be
> -		 * call if this function return 0).
> -		 */
> -		range->hmm = hmm;
> -	}
> +	} while (start < range->end);
>  
>  	return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
>  }
> -- 
> 2.17.2
>