search for: out_free_pfns

...t amdgpu_bo *bo, struct page **pages) > { > down_read(&mm->mmap_sem); > r = hmm_range_fault(range, 0); > up_read(&mm->mmap_sem); > if (unlikely(r <= 0)) { > if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) > goto retry; > goto out_free_pfns; > } > > for (i = 0; i < ttm->num_pages; i++) { > pages[i] = hmm_device_entry_to_page(range, range->pfns[i]); > > It is not allowed to read the results of hmm_range_fault() outside > locking, and in particular, we can't convert to a struct page. > > Th...

...em); >> r = hmm_range_fault(range, 0); >> up_read(&mm->mmap_sem); >> - >> - if (unlikely(r < 0)) >> + if (unlikely(r <= 0)) { >> + if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) >> + goto retry; >> goto out_free_pfns; >> + } > > This isn't really right, a retry loop like this needs to go all the > way to mmu_range_read_retry() and done under the notifier_lock. ie > mmu_range_read_retry() can fail just as likely as hmm_range_fault() > can, and drivers are supposed to retry in both cases...

...N)) { - /* - * return -EAGAIN, mmap_sem is dropped - */ - if (retry++ < MAX_RETRY_HMM_RANGE_FAULT) - goto retry; - else - pr_err("Retry hmm fault too many times\n"); - } - - goto out_up_read; - } - up_read(&mm->mmap_sem); + if (unlikely(r < 0)) + goto out_free_pfns; + for (i = 0; i < ttm->num_pages; i++) { pages[i] = hmm_device_entry_to_page(range, pfns[i]); if (unlikely(!pages[i])) { @@ -864,9 +850,6 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) return 0; -out_up_read: - if (likely(r != -EAGAIN)) - up_rea...

On Tue, Oct 29, 2019 at 07:22:37PM +0000, Yang, Philip wrote: > Hi Jason, > > I did quick test after merging amd-staging-drm-next with the > mmu_notifier branch, which includes this set changes. The test result > has different failures, app stuck intermittently, GUI no display etc. I > am understanding the changes and will try to figure out the cause. Thanks! I'm not

...> + down_read(&mm->mmap_sem); > r = hmm_range_fault(range, 0); > up_read(&mm->mmap_sem); > - > - if (unlikely(r < 0)) > + if (unlikely(r <= 0)) { > + if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) > + goto retry; > goto out_free_pfns; > + } This isn't really right, a retry loop like this needs to go all the way to mmu_range_read_retry() and done under the notifier_lock. ie mmu_range_read_retry() can fail just as likely as hmm_range_fault() can, and drivers are supposed to retry in both cases, with a single timeout. AFA...

...npages); - args.end = start + (c << PAGE_SHIFT); + pfns = nouveau_pfns_alloc(max); + if (!pfns) + goto out_free_dma; + + for (i = 0; i < npages; i += max) { + args.end = start + (max << PAGE_SHIFT); ret = migrate_vma_setup(&args); if (ret) - goto out_free_dma; + goto out_free_pfns; if (args.cpages) - nouveau_dmem_migrate_chunk(&args, drm, dma_addrs); + nouveau_dmem_migrate_chunk(&args, drm, dma_addrs, + pfns); args.start = args.end; } ret = 0; +out_free_pfns: + nouveau_pfns_free(pfns); out_free_dma: kfree(dma_addrs); out_free_dst: diff -...

...ANGE_DEFAULT_TIMEOUT); > + range->notifier_seq = mmu_range_read_begin(&bo->notifier); > > down_read(&mm->mmap_sem); > vma = find_vma(mm, start); > @@ -855,10 +861,10 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) > goto out_free_pfns; > > for (i = 0; i < ttm->num_pages; i++) { > - pages[i] = hmm_device_entry_to_page(range, pfns[i]); > + pages[i] = hmm_device_entry_to_page(range, range->pfns[i]); > if (unlikely(!pages[i])) { > pr_err("Page fault failed for pfn[%lu] = 0x%llx\n&quot...

...ange_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT); + range->notifier_seq = mmu_range_read_begin(&bo->notifier); down_read(&mm->mmap_sem); vma = find_vma(mm, start); @@ -855,10 +861,10 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) goto out_free_pfns; for (i = 0; i < ttm->num_pages; i++) { - pages[i] = hmm_device_entry_to_page(range, pfns[i]); + pages[i] = hmm_device_entry_to_page(range, range->pfns[i]); if (unlikely(!pages[i])) { pr_err("Page fault failed for pfn[%lu] = 0x%llx\n", - i, pfns[i]); +...

...up_read(&mm->mmap_sem); - - if (unlikely(r < 0)) + if (unlikely(r <= 0)) { + /* + * FIXME: This timeout should encompass the retry from + * mmu_interval_read_retry() as well. + */ + if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) + goto retry; goto out_free_pfns; + } for (i = 0; i < ttm->num_pages; i++) { - pages[i] = hmm_device_entry_to_page(range, pfns[i]); + /* FIXME: The pages cannot be touched outside the notifier_lock */ + pages[i] = hmm_device_entry_to_page(range, range->pfns[i]); if (unlikely(!pages[i])) { pr_err("Page f...

...IFT); > + pfns = nouveau_pfns_alloc(max); > + if (!pfns) > + goto out_free_dma; > + > + for (i = 0; i < npages; i += max) { > + args.end = start + (max << PAGE_SHIFT); > ret = migrate_vma_setup(&args); > if (ret) > - goto out_free_dma; > + goto out_free_pfns; > > if (args.cpages) > - nouveau_dmem_migrate_chunk(&args, drm, dma_addrs); > + nouveau_dmem_migrate_chunk(&args, drm, dma_addrs, > + pfns); > args.start = args.end; > } > > ret = 0; > +out_free_pfns: > + nouveau_pfns_free(pfns); &...

...d(&mm->mmap_sem); > > @@ -865,15 +866,19 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) > } > > gtt->range = range; > + mmput(mm); > > return 0; > > +out_unlock: > + up_read(&mm->mmap_sem); > out_free_pfns: > hmm_range_unregister(range); > kvfree(pfns); > out_free_ranges: > kfree(range); > out: > + mmput(mm); > return r; > } >

...int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) { down_read(&mm->mmap_sem); r = hmm_range_fault(range, 0); up_read(&mm->mmap_sem); if (unlikely(r <= 0)) { if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) goto retry; goto out_free_pfns; } for (i = 0; i < ttm->num_pages; i++) { pages[i] = hmm_device_entry_to_page(range, range->pfns[i]); It is not allowed to read the results of hmm_range_fault() outside locking, and in particular, we can't convert to a struct page. This must be done inside the notifier_lock, aft...

...> + down_read(&mm->mmap_sem); > r = hmm_range_fault(range, 0); > up_read(&mm->mmap_sem); > - > - if (unlikely(r < 0)) > + if (unlikely(r <= 0)) { > + if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) > + goto retry; > goto out_free_pfns; > + } I was reflecting on why this suddently became necessary, and I think what might be happening is that hmm_range_fault() is trigging invalidations as it runs (ie it is faulting in pages or something) and that in turn causes the mrn to need retry. The hmm version of this had a bug where a...

...npages); - args.end = start + (c << PAGE_SHIFT); + pfns = nouveau_pfns_alloc(max); + if (!pfns) + goto out_free_dma; + + for (i = 0; i < npages; i += max) { + args.end = start + (max << PAGE_SHIFT); ret = migrate_vma_setup(&args); if (ret) - goto out_free_dma; + goto out_free_pfns; if (args.cpages) - nouveau_dmem_migrate_chunk(drm, &args, dma_addrs); + nouveau_dmem_migrate_chunk(drm, &args, dma_addrs, + pfns); args.start = args.end; } ret = 0; +out_free_pfns: + nouveau_pfns_free(pfns); out_free_dma: kfree(dma_addrs); out_free_dst: diff -...

...+ goto out_unlock; + } + r = hmm_range_fault(range, 0); up_read(&mm->mmap_sem); @@ -865,15 +866,19 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages) } gtt->range = range; + mmput(mm); return 0; +out_unlock: + up_read(&mm->mmap_sem); out_free_pfns: hmm_range_unregister(range); kvfree(pfns); out_free_ranges: kfree(range); out: + mmput(mm); return r; } -- 2.23.0

Originally patch 4 was targeted for Jason's rdma tree since other HMM related changes were queued there. Now that those have been merged, these patches just contain changes to nouveau so they could go through any tree. I guess Ben Skeggs' tree would be appropriate. Changes since v2: Added patches 1-3 to fix some minor issues. Eliminated nouveau_find_svmm() since it is easily found.

...0)) { + if (unlikely(r)) { /* * FIXME: This timeout should encompass the retry from * mmu_interval_read_retry() as well. */ - if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) + if ((r == -EBUSY) && !time_after(jiffies, timeout)) goto retry; goto out_free_pfns; } diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 645fedd77e21b4..c68e9317cf0740 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -536,7 +536,7 @@ static int nouveau_range_fault(struct nouveau_svmm...

...;= 0)) { + if (unlikely(r)) { /* * FIXME: This timeout should encompass the retry from * mmu_interval_read_retry() as well. */ - if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout)) + if (r == -EBUSY && !time_after(jiffies, timeout)) goto retry; goto out_free_pfns; } diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c index 645fedd77e21b4..c68e9317cf0740 100644 --- a/drivers/gpu/drm/nouveau/nouveau_svm.c +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c @@ -536,7 +536,7 @@ static int nouveau_range_fault(struct nouveau_svmm...

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:

From: Jason Gunthorpe <jgg at mellanox.com> The API is a bit complicated for the uses we actually have, and disucssions for simplifying have come up a number of times. This small series removes the customizable pfn format and simplifies the return code of hmm_range_fault() All the drivers are adjusted to process in the simplified format. I would appreciated tested-by's for the two