On Mon, Mar 16, 2020 at 10:02:50AM +0100, Christoph Hellwig wrote:
> On Wed, Mar 11, 2020 at 03:35:00PM -0300, Jason Gunthorpe wrote:
> > @@ -694,6 +672,15 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
> > return -EBUSY;
> > ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
> > &hmm_walk_ops, &hmm_vma_walk);
> > + /*
> > + * A pgmap is kept cached in the hmm_vma_walk to avoid expensive
> > + * searching in the probably common case that the pgmap is the
> > + * same for the entire requested range.
> > + */
> > + if (hmm_vma_walk.pgmap) {
> > + put_dev_pagemap(hmm_vma_walk.pgmap);
> > + hmm_vma_walk.pgmap = NULL;
> > + }
> > } while (ret == -EBUSY);
>
> In which case it should only be put on return, and not for every loop.
I chose this to be simple without having to goto unwind it.
So, instead like this:
@@ -683,21 +661,33 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
.flags = flags,
};
struct mm_struct *mm = range->notifier->mm;
- int ret;
+ long ret;
lockdep_assert_held(&mm->mmap_sem);
do {
/* If range is no longer valid force retry. */
if (mmu_interval_check_retry(range->notifier,
- range->notifier_seq))
- return -EBUSY;
+ range->notifier_seq)) {
+ ret = -EBUSY;
+ goto out;
+ }
ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
&hmm_walk_ops, &hmm_vma_walk);
} while (ret == -EBUSY);
if (ret)
- return ret;
- return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
+ goto out;
+ ret = (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
+
+out:
+ /*
+ * A pgmap is kept cached in the hmm_vma_walk to avoid expensive
+ * searching in the probably common case that the pgmap is the
+ * same for the entire requested range.
+ */
+ if (hmm_vma_walk.pgmap)
+ put_dev_pagemap(hmm_vma_walk.pgmap);
+ return ret;
}
EXPORT_SYMBOL(hmm_range_fault);
?
> I still think the right fix is to just delete all the unused and broken
> pgmap handling code. If we ever need to add it back it can be added
> in a proper understood and tested way.
What I want to add is something like
if (pgmap != walk->required_pgmap)
cpu_flags = 0
hmm_range_need_fault(..., cpu_flags, ...)
Which will fix a bug in nouveau where it blindly assumes any device
pages are its own, IIRC.
I think Ralph observed it needs to be here, because if the pgmap
doesn't match then it should trigger migration, in a single call,
rather than iterating.
I'm mostly expecting to replace all the other pgmap code, but keep the
pgmap caching. The existing pgmap stuff seems approx right, but
useless..
Jason
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
