search for: nouveau_range_done

...019 at 03:18:28PM +0000, Jason Gunthorpe wrote: > > Hum.. > > > > The caller does this: > > > > again: > > ret = nouveau_range_fault(&svmm->mirror, &range); > > if (ret == 0) { > > mutex_lock(&svmm->mutex); > > if (!nouveau_range_done(&range)) { > > mutex_unlock(&svmm->mutex); > > goto again; > > > > And we can't call nouveau_range_fault() -> hmm_range_fault() without > > holding the mmap_sem, so we can't allow nouveau_range_fault to unlock > > it. > > Got...

...; ret = -EBUSY; > + up_read(&range->vma->vm_mm->mmap_sem); > hmm_range_unregister(range); > return ret; Hum.. The caller does this: again: ret = nouveau_range_fault(&svmm->mirror, &range); if (ret == 0) { mutex_lock(&svmm->mutex); if (!nouveau_range_done(&range)) { mutex_unlock(&svmm->mutex); goto again; And we can't call nouveau_range_fault() -> hmm_range_fault() without holding the mmap_sem, so we can't allow nouveau_range_fault to unlock it. Maybe this instead? diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c...

...changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 033a9241a14a..9a9f71e4be29 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -491,8 +491,7 @@ static inline bool nouveau_range_done(struct hmm_range *range) } static int -nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, - bool block) +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range) { long ret; @@ -510,7 +509,7 @@ nouveau_range_fault(struct hmm_mirror *mirror, struct h...

...changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index cde09003c06b..5dd83a46578f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -484,8 +484,7 @@ static inline bool nouveau_range_done(struct hmm_range *range) } static int -nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, - bool block) +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range) { long ret; @@ -503,7 +502,7 @@ nouveau_range_fault(struct hmm_mirror *mirror, struct h...

...6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index a74530b5a523..b889d5ec4c7e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -485,14 +485,14 @@ nouveau_range_done(struct hmm_range *range) } static int -nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range) +nouveau_range_fault(struct nouveau_svmm *svmm, struct hmm_range *range) { long ret; range->default_flags = 0; range->pfn_flags_mask = -1UL; - ret = hmm_range_register...

...++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index a74530b5a523..98072fd48cf7 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -485,23 +485,23 @@ nouveau_range_done(struct hmm_range *range) } static int -nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range) +nouveau_range_fault(struct nouveau_svmm *svmm, struct hmm_range *range) { long ret; range->default_flags = 0; range->pfn_flags_mask = -1UL; - ret = hmm_range_register...

.../nouveau/nouveau_svm.c index 8c92374afcf2..cde09003c06b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -475,6 +475,47 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm, fault->inst, fault->addr, fault->access); } +static inline bool nouveau_range_done(struct hmm_range *range) +{ + bool ret = hmm_range_valid(range); + + hmm_range_unregister(range); + return ret; +} + +static int +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, + bool block) +{ + long ret; + + range->default_flags = 0; + range->pfn_flags_mask = -...

Hi Jérôme, Ben and Jason, below is a series against the hmm tree which fixes up the mmap_sem locking in nouveau and while at it also removes leftover legacy HMM APIs only used by nouveau. Changes since v1: - don't return the valid state from hmm_range_unregister - additional nouveau cleanups

...long time to + * wait already. + */ +#define NOUVEAU_RANGE_FAULT_TIMEOUT 1000 + struct nouveau_svm { struct nouveau_drm *drm; struct mutex mutex; @@ -475,6 +482,47 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm, fault->inst, fault->addr, fault->access); } +static inline bool nouveau_range_done(struct hmm_range *range) +{ + bool ret = hmm_range_valid(range); + + hmm_range_unregister(range); + return ret; +} + +static int +nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range, + bool block) +{ + long ret; + + range->default_flags = 0; + range->pfn_flags_mask = -...

...ref. > > As long take_driver_page_table_lock() in Jerome's flow can replace > percpu_ref_tryget_live() on the pagemap reference. It seems > nouveau_dmem_convert_pfn() happens after: > > mutex_lock(&svmm->mutex); > if (!nouveau_range_done(&range)) { > > ...so I would expect that to be functionally equivalent to validating > the reference count. Yes, OK, that makes sense, I was mostly surprised by the statement the driver doesn't touch the struct page.. I suppose "doesn't touch the struct page out of the...

Hi Jérôme, Ben and Jason, below is a series against the hmm tree which fixes up the mmap_sem locking in nouveau and while at it also removes leftover legacy HMM APIs only used by nouveau. The first 4 patches are a bug fix for nouveau, which I suspect should go into this merge window even if the code is marked as staging, just to avoid people copying the breakage. Changes since v2: - new patch

Hi Jérôme, Ben and Jason, below is a series against the hmm tree which fixes up the mmap_sem locking in nouveau and while at it also removes leftover legacy HMM APIs only used by nouveau. The first 4 patches are a bug fix for nouveau, which I suspect should go into this merge window even if the code is marked as staging, just to avoid people copying the breakage. Changes since v1: - don't

On Thu, Aug 15, 2019 at 04:33:06PM -0400, Jerome Glisse wrote: > So nor HMM nor driver should dereference the struct page (i do not > think any iommu driver would either), Er, they do technically deref the struct page: nouveau_dmem_convert_pfn(struct nouveau_drm *drm, struct hmm_range *range) struct page *page; page = hmm_pfn_to_page(range, range->pfns[i]); if

...rn 0; -out_hmm_unregister: - hmm_mirror_unregister(&svmm->mirror); out_mm_unlock: up_write(&current->mm->mmap_sem); out_free: @@ -503,43 +482,89 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm, fault->inst, fault->addr, fault->access); } -static inline bool -nouveau_range_done(struct hmm_range *range) +struct svm_notifier { + struct mmu_range_notifier notifier; + struct nouveau_svmm *svmm; +}; + +static bool nouveau_svm_range_invalidate(struct mmu_range_notifier *mrn, + const struct mmu_notifier_range *range) { - bool ret = hmm_range_valid(range); + struct svm_noti...

On Tue, Jul 23, 2019 at 03:18:28PM +0000, Jason Gunthorpe wrote: > Hum.. > > The caller does this: > > again: > ret = nouveau_range_fault(&svmm->mirror, &range); > if (ret == 0) { > mutex_lock(&svmm->mutex); > if (!nouveau_range_done(&range)) { > mutex_unlock(&svmm->mutex); > goto again; > > And we can't call nouveau_range_fault() -> hmm_range_fault() without > holding the mmap_sem, so we can't allow nouveau_range_fault to unlock > it. Goto again can only happen if nouveau_range...

On Tue, Jul 23, 2019 at 02:17:31PM -0300, Jason Gunthorpe wrote: > That reminds me, this code is also leaking hmm_range_unregister() in > the success path, right? No, that is done by hmm_vma_range_done / nouveau_range_done for the success path. > > I think the right way to structure this is to move the goto again and > related into the nouveau_range_fault() so the whole retry algorithm is > sensibly self contained. Then we'd take svmm->mutex inside the helper and let the caller unlock that. Eit...

...alidation while doing the above deref. As long take_driver_page_table_lock() in Jerome's flow can replace percpu_ref_tryget_live() on the pagemap reference. It seems nouveau_dmem_convert_pfn() happens after: mutex_lock(&svmm->mutex); if (!nouveau_range_done(&range)) { ...so I would expect that to be functionally equivalent to validating the reference count.

...ake_driver_page_table_lock() in Jerome's flow can replace > > percpu_ref_tryget_live() on the pagemap reference. It seems > > nouveau_dmem_convert_pfn() happens after: > > > > mutex_lock(&svmm->mutex); > > if (!nouveau_range_done(&range)) { > > > > ...so I would expect that to be functionally equivalent to validating > > the reference count. > > Yes, OK, that makes sense, I was mostly surprised by the statement the > driver doesn't touch the struct page.. > > I suppose "doesn...

Hi Jérôme, Ben, Felix and Jason, below is a series against the hmm tree which cleans up various minor bits and allows HMM_MIRROR to be built on all architectures. Diffstat: 11 files changed, 94 insertions(+), 210 deletions(-) A git tree is also available at: git://git.infradead.org/users/hch/misc.git hmm-cleanups.2 Gitweb:

Hi Jérôme, Ben, Felxi and Jason, below is a series against the hmm tree which cleans up various minor bits and allows HMM_MIRROR to be built on all architectures. Diffstat: 7 files changed, 81 insertions(+), 171 deletions(-) A git tree is also available at: git://git.infradead.org/users/hch/misc.git hmm-cleanups Gitweb: