Re: [PATCH v3 3/3] mm/hmm/test: add self tests for HMM

[Ralph Campbell <rcampbell@nvidia.com>]

On 10/29/19 10:58 AM, Jason Gunthorpe wrote:
> On Wed, Oct 23, 2019 at 12:55:15PM -0700, Ralph Campbell wrote:
>> Add self tests for HMM.
>>
>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
> 
>> ---
>>   MAINTAINERS                            |    3 +
>>   drivers/char/Kconfig                   |   11 +
>>   drivers/char/Makefile                  |    1 +
>>   drivers/char/hmm_dmirror.c             | 1566 ++++++++++++++++++++++++
>>   include/Kbuild                         |    1 +
>>   include/uapi/linux/hmm_dmirror.h       |   74 ++
>>   tools/testing/selftests/vm/.gitignore  |    1 +
>>   tools/testing/selftests/vm/Makefile    |    3 +
>>   tools/testing/selftests/vm/config      |    3 +
>>   tools/testing/selftests/vm/hmm-tests.c | 1311 ++++++++++++++++++++
>>   tools/testing/selftests/vm/run_vmtests |   16 +
>>   tools/testing/selftests/vm/test_hmm.sh |   97 ++
>>   12 files changed, 3087 insertions(+)
>>   create mode 100644 drivers/char/hmm_dmirror.c
>>   create mode 100644 include/uapi/linux/hmm_dmirror.h
>>   create mode 100644 tools/testing/selftests/vm/hmm-tests.c
>>   create mode 100755 tools/testing/selftests/vm/test_hmm.sh
> 
> This is really big, it would be nice to get a comment from the various
> kernel testing folks if this approach makes sense with the test
> frameworks. Do we have other drivers that are only intended to be used
> by selftests?
> 
> Frankly, I'm not super excited about the idea of a 'test driver', it
> seems more logical for testing to have some way for a test harness to
> call hmm_range_fault() under various conditions and check the results?

test_vmalloc.sh at least uses a test module(s).

> It seems especially over-complicated to use a full page table layout
> for this, wouldn't something simple like an xarray be good enough for
> test purposes?

Possibly. A page table is really just a lookup table from virtual address
to pfn/page. Part of the rationale was to mimic what a real device might do.


>> +/*
>> + * Below are the file operation for the dmirror device file. Only ioctl matters.
>> + *
>> + * Note this is highly specific to the dmirror device driver and should not be
>> + * construed as an example on how to design the API a real device driver would
>> + * expose to userspace.
>> + */
>> +static ssize_t dmirror_fops_read(struct file *filp,
>> +			       char __user *buf,
>> +			       size_t count,
>> +			       loff_t *ppos)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static ssize_t dmirror_fops_write(struct file *filp,
>> +				const char __user *buf,
>> +				size_t count,
>> +				loff_t *ppos)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int dmirror_fops_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> +	/* Forbid mmap of the dmirror device file. */
>> +	return -EINVAL;
>> +}
> 
> I'm pretty sure these can just be left as NULL in the fops?

I think so.

>> +static int dmirror_fault(struct dmirror *dmirror,
>> +			 unsigned long start,
>> +			 unsigned long end,
>> +			 bool write)
>> +{
>> +	struct mm_struct *mm = dmirror->mirror.hmm->mmu_notifier.mm;
>> +	unsigned long addr;
>> +	unsigned long next;
>> +	uint64_t pfns[64];
>> +	struct hmm_range range = {
>> +		.pfns = pfns,
>> +		.flags = dmirror_hmm_flags,
>> +		.values = dmirror_hmm_values,
>> +		.pfn_shift = DPT_SHIFT,
>> +		.pfn_flags_mask = ~(dmirror_hmm_flags[HMM_PFN_VALID] |
>> +				    dmirror_hmm_flags[HMM_PFN_WRITE]),
>> +		.default_flags = dmirror_hmm_flags[HMM_PFN_VALID] |
>> +				(write ? dmirror_hmm_flags[HMM_PFN_WRITE] : 0),
>> +	};
>> +	int ret = 0;
>> +
>> +	for (addr = start; addr < end; ) {
>> +		long count;
>> +
>> +		next = min(addr + (ARRAY_SIZE(pfns) << PAGE_SHIFT), end);
>> +		range.start = addr;
>> +		range.end = next;
>> +
>> +		down_read(&mm->mmap_sem);
>> +
>> +		ret = hmm_range_register(&range, &dmirror->mirror);
>> +		if (ret) {
>> +			up_read(&mm->mmap_sem);
>> +			break;
>> +		}
>> +
>> +		if (!hmm_range_wait_until_valid(&range,
>> +						DMIRROR_RANGE_FAULT_TIMEOUT)) {
>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			continue;
>> +		}
>> +
>> +		count = hmm_range_fault(&range, 0);
>> +		if (count < 0) {
>> +			ret = count;
>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			break;
>> +		}
>> +
>> +		if (!hmm_range_valid(&range)) {
> 
> There is no 'driver lock' being held here, how does this work?
> Shouldn't it hold dmirror->mutex for this sequence?

I have a modified version of this driver that's based on your series
removing hmm_mirror_register() which uses a mutex.
Otherwise, it looks similar to the changes in nouveau.

>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			continue;
>> +		}
>> +		mutex_lock(&dmirror->mutex);
>> +		ret = dmirror_pt_walk(dmirror, dmirror_do_fault,
>> +				      addr, next, &range, true);
>> +		mutex_unlock(&dmirror->mutex);
> 
> Ie move it down into this block
> 
>> +		hmm_range_unregister(&range);
>> +		up_read(&mm->mmap_sem);
>> +		if (ret)
>> +			break;
>> +
>> +		addr = next;
>> +	}
>> +
>> +	return ret;
>> +}
> 
>> +static int dmirror_read(struct dmirror *dmirror,
>> +			struct hmm_dmirror_cmd *cmd)
>> +{
> 
> Why not just use pread()/pwrite() for this instead of an ioctl?

pread()/pwrite() could certainly be implemented.
I think the idea was that the read/write is actually the "device"
doing read/write and making that clearly different from a program
reading/writing the device. Also, the ioctl() allows information
about what faults or events happened during the operation. I only
have number of pages and number of page faults returned at the moment,
but one of Jerome's version of this driver had other counters being
returned.

>> +	struct dmirror_bounce bounce;
>> +	unsigned long start, end;
>> +	unsigned long size = cmd->npages << PAGE_SHIFT;
>> +	int ret;
>> +
>> +	start = cmd->addr;
>> +	end = start + size;
>> +	if (end < start)
>> +		return -EINVAL;
>> +
>> +	ret = dmirror_bounce_init(&bounce, start, size);
>> +	if (ret)
>> +		return ret;
>> +
>> +static int dmirror_snapshot(struct dmirror *dmirror,
>> +			    struct hmm_dmirror_cmd *cmd)
>> +{
>> +	struct mm_struct *mm = dmirror->mirror.hmm->mmu_notifier.mm;
>> +	unsigned long start, end;
>> +	unsigned long size = cmd->npages << PAGE_SHIFT;
>> +	unsigned long addr;
>> +	unsigned long next;
>> +	uint64_t pfns[64];
>> +	unsigned char perm[64];
>> +	char __user *uptr;
>> +	struct hmm_range range = {
>> +		.pfns = pfns,
>> +		.flags = dmirror_hmm_flags,
>> +		.values = dmirror_hmm_values,
>> +		.pfn_shift = DPT_SHIFT,
>> +		.pfn_flags_mask = ~0ULL,
>> +	};
>> +	int ret = 0;
>> +
>> +	start = cmd->addr;
>> +	end = start + size;
>> +	uptr = (void __user *)cmd->ptr;
>> +
>> +	for (addr = start; addr < end; ) {
>> +		long count;
>> +		unsigned long i;
>> +		unsigned long n;
>> +
>> +		next = min(addr + (ARRAY_SIZE(pfns) << PAGE_SHIFT), end);
>> +		range.start = addr;
>> +		range.end = next;
>> +
>> +		down_read(&mm->mmap_sem);
>> +
>> +		ret = hmm_range_register(&range, &dmirror->mirror);
>> +		if (ret) {
>> +			up_read(&mm->mmap_sem);
>> +			break;
>> +		}
>> +
>> +		if (!hmm_range_wait_until_valid(&range,
>> +						DMIRROR_RANGE_FAULT_TIMEOUT)) {
>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			continue;
>> +		}
>> +
>> +		count = hmm_range_fault(&range, HMM_FAULT_SNAPSHOT);
>> +		if (count < 0) {
>> +			ret = count;
>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			if (ret == -EBUSY)
>> +				continue;
>> +			break;
>> +		}
>> +
>> +		if (!hmm_range_valid(&range)) {
> 
> Same as for dmirror_fault
> 
>> +			hmm_range_unregister(&range);
>> +			up_read(&mm->mmap_sem);
>> +			continue;
>> +		}
>> +
>> +		n = (next - addr) >> PAGE_SHIFT;
>> +		for (i = 0; i < n; i++)
>> +			dmirror_mkentry(dmirror, &range, perm + i, pfns[i]);
> 
> Is this missing locking too?

Yes. It's in the updated version as mentioned above.

>> +static int dmirror_remove(struct platform_device *pdev)
>> +{
>> +	/* all probe actions are unwound by devm */
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver dmirror_device_driver = {
>> +	.probe		= dmirror_probe,
>> +	.remove		= dmirror_remove,
>> +	.driver		= {
>> +		.name	= "HMM_DMIRROR",
>> +	},
>> +};
> 
> This presence of a platform_driver and device is very confusing. I'm
> sure Greg KH would object to this as a misuse of platform drivers.
> 
> A platform device isn't needed to create a char dev, so what is this for?

The devm_request_free_mem_region() and devm_memremap_pages() calls for
creating the ZONE_DEVICE private pages tie into the devm* clean up framework.
I thought a platform_driver was the simplest way to also be able to call
devm_add_action_or_reset() to clean up on module unload and be compatible
with the private page clean up.

>> diff --git a/include/Kbuild b/include/Kbuild
>> index ffba79483cc5..6ffb44a45957 100644
>> --- a/include/Kbuild
>> +++ b/include/Kbuild
>> @@ -1063,6 +1063,7 @@ header-test-			+= uapi/linux/coda_psdev.h
>>   header-test-			+= uapi/linux/errqueue.h
>>   header-test-			+= uapi/linux/eventpoll.h
>>   header-test-			+= uapi/linux/hdlc/ioctl.h
>> +header-test-			+= uapi/linux/hmm_dmirror.h
> 
> Why? This list should only be updated if the header is broken in some
> way.

Should this be in include/linux/ instead?
I wasn't sure where the "right" place was to put the header.

> 
>> diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c
>> new file mode 100644
>> index 000000000000..f4ae6188fd0e
>> --- /dev/null
>> +++ b/tools/testing/selftests/vm/hmm-tests.c
>> @@ -0,0 +1,1311 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2013 Red Hat Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
> 
> btw, I think if a SPDX is present I don't think the license text is
> required, just the copyright.

Since I was starting from Jerome's HMM test driver, I didn't want to
delete any of the original copyright text.
If Jerome is OK with just the SPDX header, that's OK with me.

> I think these tests should also study the various case of invoke
> pte_hole, ie faulting/snappshotting before/after a vma, or across a
> vma range with a hole, etc, etc.
> 
> Jason
> 

There are tests for vma hole, pte_none(), zero page, and normal page.
Nothing stress testing races, just set up the mmap() and test once.
I can add more test cases if you have something specific in mind.