[1/5] mm: return valid info from hmm_range_unregister

Submitted by Christoph Hellwig on July 3, 2019, 6:44 p.m.

Details

Message ID 20190703184502.16234-2-hch@lst.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Christoph Hellwig July 3, 2019, 6:44 p.m.
Checking range->valid is trivial and has no meaningful cost, but
nicely simplifies the fastpath in typical callers.  Also remove the
hmm_vma_range_done function, which now is a trivial wrapper around
hmm_range_unregister.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
---
 drivers/gpu/drm/nouveau/nouveau_svm.c |  2 +-
 include/linux/hmm.h                   | 11 +----------
 mm/hmm.c                              |  7 ++++++-
 3 files changed, 8 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 8c92374afcf2..9d40114d7949 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -652,7 +652,7 @@  nouveau_svm_fault(struct nvif_notify *notify)
 		ret = hmm_vma_fault(&svmm->mirror, &range, true);
 		if (ret == 0) {
 			mutex_lock(&svmm->mutex);
-			if (!hmm_vma_range_done(&range)) {
+			if (!hmm_range_unregister(&range)) {
 				mutex_unlock(&svmm->mutex);
 				goto again;
 			}
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index b8a08b2a10ca..6b55e59fd8e3 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -462,7 +462,7 @@  int hmm_range_register(struct hmm_range *range,
 		       unsigned long start,
 		       unsigned long end,
 		       unsigned page_shift);
-void hmm_range_unregister(struct hmm_range *range);
+bool 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);
 long hmm_range_dma_map(struct hmm_range *range,
@@ -484,15 +484,6 @@  long hmm_range_dma_unmap(struct hmm_range *range,
  */
 #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;
-}
-
 /* This is a temporary helper to avoid merge conflict between trees. */
 static inline int hmm_vma_fault(struct hmm_mirror *mirror,
 				struct hmm_range *range, bool block)
diff --git a/mm/hmm.c b/mm/hmm.c
index d48b9283725a..ac238d3f1f4e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -917,11 +917,15 @@  EXPORT_SYMBOL(hmm_range_register);
  *
  * Range struct is used to track updates to the CPU page table after a call to
  * hmm_range_register(). See include/linux/hmm.h for how to use it.
+ *
+ * Return:	%true if the range was still valid at the time of unregistering,
+ *		else %false.
  */
-void hmm_range_unregister(struct hmm_range *range)
+bool hmm_range_unregister(struct hmm_range *range)
 {
 	struct hmm *hmm = range->hmm;
 	unsigned long flags;
+	bool ret = range->valid;
 
 	spin_lock_irqsave(&hmm->ranges_lock, flags);
 	list_del_init(&range->list);
@@ -938,6 +942,7 @@  void hmm_range_unregister(struct hmm_range *range)
 	 */
 	range->valid = false;
 	memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
+	return ret;
 }
 EXPORT_SYMBOL(hmm_range_unregister);
 

Comments

On Wed, Jul 03, 2019 at 11:44:58AM -0700, Christoph Hellwig wrote:
> Checking range->valid is trivial and has no meaningful cost, but
> nicely simplifies the fastpath in typical callers.  

It should not be the typical caller..

> hmm_vma_range_done function, which now is a trivial wrapper around
> hmm_range_unregister.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
>  drivers/gpu/drm/nouveau/nouveau_svm.c |  2 +-
>  include/linux/hmm.h                   | 11 +----------
>  mm/hmm.c                              |  7 ++++++-
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 8c92374afcf2..9d40114d7949 100644
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -652,7 +652,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
>  		ret = hmm_vma_fault(&svmm->mirror, &range, true);
>  		if (ret == 0) {
>  			mutex_lock(&svmm->mutex);
> -			if (!hmm_vma_range_done(&range)) {
> +			if (!hmm_range_unregister(&range)) {
>  				mutex_unlock(&svmm->mutex);
>  				goto again;
>  			}

In this case if we take the 'goto again' then we are pointlessly
removing and re-adding the range.

The pattern is supposed to be:

    hmm_range_register()
again:
    .. read page tables ..
    lock
    if (!hmm_range_valid())
        unlock
        goto again
    .. setup device ..
    unlock
    hmm_range_unregister()

I don't think the API should be encouraging some shortcut here..

We can't do the above pattern because the old hmm_vma API didn't allow
it, which is presumably a reason why it is obsolete.

I'd rather see drivers move to a consistent pattern so we can then
easily hoist the seqcount lock scheme into some common mmu notifier
code, as discussed.

Jason
On Wed, Jul 03, 2019 at 07:00:50PM +0000, Jason Gunthorpe wrote:
> I don't think the API should be encouraging some shortcut here..
> 
> We can't do the above pattern because the old hmm_vma API didn't allow
> it, which is presumably a reason why it is obsolete.
> 
> I'd rather see drivers move to a consistent pattern so we can then
> easily hoist the seqcount lock scheme into some common mmu notifier
> code, as discussed.

So you don't like the version in amdgpu_ttm_tt_get_user_pages_done in
linux-next either?

I can remove this and just move hmm_vma_range_done to nouveau instead.
Let me know if you have other comments before I resend.  Note that
I'll probably be offline Thu-Sun this week.
On Wed, Jul 03, 2019 at 10:28:57PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 03, 2019 at 07:00:50PM +0000, Jason Gunthorpe wrote:
> > I don't think the API should be encouraging some shortcut here..
> > 
> > We can't do the above pattern because the old hmm_vma API didn't allow
> > it, which is presumably a reason why it is obsolete.
> > 
> > I'd rather see drivers move to a consistent pattern so we can then
> > easily hoist the seqcount lock scheme into some common mmu notifier
> > code, as discussed.
> 
> So you don't like the version in amdgpu_ttm_tt_get_user_pages_done in
> linux-next either?

I looked at this for 5 mins, and I can't see the key elements of the
collision retry lock:

- Where is the retry loop?
- Where is the lock around the final test to valid prior to using
  the output of range?

For instance looking at amdgpu_gem_userptr_ioctl()..

We can't be holding a lock when we do hmm_range_wait_until_valid()
(inside amdgpu_ttm_tt_get_user_pages), otherwise it deadlocks, and
there are not other locks that would encompass the final is_valid check.

And amdgpu_gem_userptr_ioctl() looks like a syscall entry point, so
having it fail just because the lock collided (ie is_valid == false)
can't possibly be the right thing.

I'm also unclear when the device data is updated in that sequence..

So.. I think this locking is wrong. Maybe AMD team can explain how it
should work?

Jason