HMM tests and minor fixes

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Although we could clean this up a tidbit more by removing the start variable:

mm/hmm.c +1 -2

index d4984a08ed9b..b5b1ed646c2f 100644
@@ -667,10 +667,9 @@ static const struct mm_walk_ops hmm_walk_ops = {
   */
  long hmm_range_fault(struct hmm_range *range, unsigned int flags)
  {
-	unsigned long start = range->start;
  	struct hmm_vma_walk hmm_vma_walk = {
  		.range = range,
-		.last = start,
+		.last = range->start,
  		.flags = flags,
  	};
  	struct mm_struct *mm = range->notifier->mm;

@@ -682,9 +681,8 @@ long hmm_range_fault(struct hmm_range *range, unsigned int flags)
  		/* If range is no longer valid force retry. */

hmm_range_fault() calls find_vma() and walk_page_range() in a loop. This is unnecessary duplication since walk_page_range() calls find_vma() in a loop already.
Simplify hmm_range_fault() by defining a walk_test() callback function to filter unhandled vmas.
This also fixes a bug where hmm_range_fault() was not checking start >= vma->vm_start before checking vma->vm_flags so hmm_range_fault() could return an error based on the wrong vma for the requested range. It also fixes a bug when the vma has no read access and the caller did not request a fault, there shouldn't be any error return code.

Signed-off-by Ralph Campbell <[email protected]>

Cc "Jérôme Glisse" <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Christoph Hellwig <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 mm/hmm.c | 121 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 63 deletions(-)

lib/test_hmm.c new file +8 -0

+static int dmirror_bounce_copy_from(struct dmirror_bounce *bounce,
+				    unsigned long addr)
+{
+	unsigned long end = addr + bounce->size;
+	char __user *uptr = (void __user *)addr;
+	void *ptr = bounce->ptr;
+
+	for (; addr < end; addr += PAGE_SIZE, ptr += PAGE_SIZE,

lib/test_hmm.c new file +4 -0

+static int dmirror_invalidate_range_start(struct mmu_notifier *mn,
+				const struct mmu_notifier_range *update)
+{
+	struct dmirror *dmirror = container_of(mn, struct dmirror, notifier);

Shouldn't this go into mm/ instead? It certainly doesn't seem like a library.

Quoted 13 lines

lib/test_hmm.c new file +13 -0

+static int dmirror_bounce_copy_from(struct dmirror_bounce *bounce,
+				    unsigned long addr)
+{
+	unsigned long end = addr + bounce->size;
+	char __user *uptr = (void __user *)addr;
+	void *ptr = bounce->ptr;
+
+	for (; addr < end; addr += PAGE_SIZE, ptr += PAGE_SIZE,
+					      uptr += PAGE_SIZE) {
+		int ret;
+
+		ret = copy_from_user(ptr, uptr, PAGE_SIZE);
+		if (ret)

lib/test_hmm.c line 145

Quoted 5 lines

lib/test_hmm.c new file +5 -0

+			return ret;
+	}
+
+	return 0;
+}

Why does this iterate in page sized chunks? I don't remember a page size limit on copy_{from,to}_user.

Quoted 8 lines

lib/test_hmm.c new file +8 -0

+static int dmirror_invalidate_range_start(struct mmu_notifier *mn,
+				const struct mmu_notifier_range *update)
+{
+	struct dmirror *dmirror = container_of(mn, struct dmirror, notifier);
+
+	if (mmu_notifier_range_blockable(update))
+		mutex_lock(&dmirror->mutex);
+	else if (!mutex_trylock(&dmirror->mutex))

lib/test_hmm.c line 198

Quoted 6 lines

lib/test_hmm.c new file +6 -0

+		return -EAGAIN;
+
+	dmirror_do_update(dmirror, update->start, update->end);
+	mutex_unlock(&dmirror->mutex);
+	return 0;
+}

Can we adopts this to Jasons new interval tree invalidate?

Quoted 4 lines

lib/test_hmm.c new file +4 -0

+static int dmirror_fops_open(struct inode *inode, struct file *filp)
+{
+	struct cdev *cdev = inode->i_cdev;
+	struct dmirror_device *mdevice;

lib/test_hmm.c line 254

Quoted 5 lines

lib/test_hmm.c new file +5 -0

+	struct dmirror *dmirror;
+
+	/* No exclusive opens. */
+	if (filp->f_flags & O_EXCL)
+		return -EINVAL;

Device files usually just ignore O_EXCL, I don't see why this one would be any different.

lib/test_hmm.c line 266

Quoted 11 lines

lib/test_hmm.c new file +11 -0

+	mdevice = container_of(cdev, struct dmirror_device, cdevice);
+	dmirror = dmirror_new(mdevice);
+	if (!dmirror)
+		return -ENOMEM;
+
+	/* Only the first open registers the address space. */
+	mutex_lock(&mdevice->devmem_lock);
+	if (filp->private_data)
+		goto err_busy;
+	filp->private_data = dmirror;
+	mutex_unlock(&mdevice->devmem_lock);

->open is only called for the first open of a given file structure..

lib/test_hmm.c line 281

Quoted 6 lines

lib/test_hmm.c new file +6 -0

+static int dmirror_fops_release(struct inode *inode, struct file *filp)
+{
+	struct dmirror *dmirror = filp->private_data;
+
+	if (!dmirror)
+		return 0;

This can't happen if your ->open never returns 0 without setting the private data.

lib/test_hmm.c line 283

Quoted 1 line

lib/test_hmm.c new file +1 -0

+	filp->private_data = NULL;

The file is feed afterwards, no need to clear the private data.

Quoted 8 lines

Quoted 8 lines

lib/test_hmm.c new file +8 -0

+static int dmirror_invalidate_range_start(struct mmu_notifier *mn,
+				const struct mmu_notifier_range *update)
+{
+	struct dmirror *dmirror = container_of(mn, struct dmirror, notifier);
+
+	if (mmu_notifier_range_blockable(update))
+		mutex_lock(&dmirror->mutex);
+	else if (!mutex_trylock(&dmirror->mutex))

Quoted 8 lines

lib/test_hmm.c line 198

Quoted 6 lines

lib/test_hmm.c new file +6 -0

+		return -EAGAIN;
+
+	dmirror_do_update(dmirror, update->start, update->end);
+	mutex_unlock(&dmirror->mutex);
+	return 0;
+}

Can we adopts this to Jasons new interval tree invalidate?

Well, it would mean registering for the whole process address space. I'll give it a try.

Quoted 2 lines

Well, it would mean registering for the whole process address space. I'll give it a try.

I'm not sure it makes much sense that this testing is essentially modeled after nouveau's usage which is very strange compared to the other drivers.

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines

Well, it would mean registering for the whole process address space. I'll give it a try.

I'm not sure it makes much sense that this testing is essentially modeled after nouveau's usage which is very strange compared to the other drivers.

Which means we really should make the test cases fit the proper usage. Maybe defer the tests for 5.5 and just merge the first patch for now?

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines

Well, it would mean registering for the whole process address space. I'll give it a try.

I'm not sure it makes much sense that this testing is essentially modeled after nouveau's usage which is very strange compared to the other drivers.

Which means we really should make the test cases fit the proper usage. Maybe defer the tests for 5.5 and just merge the first patch for now?

I think this a good point to discuss.
Some devices will want to register for all changes to the process address space because there is no requirement to preregister regions that the device can access verses devices like InfiniBand where a range of addresses have to be registered before the device can access those addresses.

Quoted 14 lines · Ralph Campbell

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines

Well, it would mean registering for the whole process address space. I'll give it a try.

I'm not sure it makes much sense that this testing is essentially modeled after nouveau's usage which is very strange compared to the other drivers.

Which means we really should make the test cases fit the proper usage. Maybe defer the tests for 5.5 and just merge the first patch for now?

Quoted 5 lines

I think this a good point to discuss.
Some devices will want to register for all changes to the process address space because there is no requirement to preregister regions that the device can access verses devices like InfiniBand where a range of addresses have to be registered before the device can access those addresses.

But this is a very bad idea to register and do HW actions for ranges that can't possibly have any pages registered. It slows down the entire application

I think the ODP approach might be saner, when it mirrors the entire address space it chops it up into VA chunks, and once a page is registered on the HW the VA chunk goes into the interval tree.

Presumably the GPU also has some kind of page table tree and you could set one of the levels as the VA interval when there are populated children

Jason

Quoted 32 lines · Jason Gunthorpe

Quoted 14 lines · Ralph Campbell

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines

Well, it would mean registering for the whole process address space. I'll give it a try.

I'm not sure it makes much sense that this testing is essentially modeled after nouveau's usage which is very strange compared to the other drivers.

Which means we really should make the test cases fit the proper usage. Maybe defer the tests for 5.5 and just merge the first patch for now?

Quoted 5 lines

I think this a good point to discuss.
Some devices will want to register for all changes to the process address space because there is no requirement to preregister regions that the device can access verses devices like InfiniBand where a range of addresses have to be registered before the device can access those addresses.

But this is a very bad idea to register and do HW actions for ranges that can't possibly have any pages registered. It slows down the entire application

I think the ODP approach might be saner, when it mirrors the entire address space it chops it up into VA chunks, and once a page is registered on the HW the VA chunk goes into the interval tree.

Presumably the GPU also has some kind of page table tree and you could set one of the levels as the VA interval when there are populated children

Quoted 1 line

Jason

I wasn't suggesting that HW invalidates happen in two places. I'm suggesting the two styles of invalidates can work together. For example, what if a driver calls mmu_notifier_register(mn, mm) to register for address space wide invalidations, then some time later there is a device page table fault and the driver calls mmu_range_notifier_insert() but with a NULL ops.invalidate.

The global invalidate() callback would get the device lock and call into mm to update the sequence number of any affected ranges instead of having a range invalidate callback, and then do the HW invalidations.

Quoted 2 lines · Christoph Hellwig

Shouldn't this go into mm/ instead? It certainly doesn't seem like a library.

I was following the convention for the other vm test kernel modules. I see a couple of modules in mm/ but I don't have a personal preference for where to place it.

Andrew, do you have a preference?