On 10/29/19 4:12 PM, Jason Gunthorpe wrote:
On Tue, Oct 29, 2019 at 02:16:05PM -0700, Ralph Campbell wrote:
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).
Well, that is good, is it also under drivers/char? It kind feels like
it should not be there...
I think most of the test modules live in lib/ but I wasn't sure that
was the right place for the HMM test driver.
If you think that is better, I can easily move it.
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.
Well, but the details of the page table layout don't see really
important to this testing, IMHO.
One problem with XArray is that on 32-bit machines the value would
need to be u64 to hold a pfn which won't fit in a ULONG_MAX.
I guess we could make the driver 64-bit only.
+ 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);
Also, did we get a mmget() before doing this down_read?
+
+ 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.
Well, that locking pattern is required even for original hmm calls..
Will be fixed in v4.
+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.
Makes sense I guess
+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.
IIRC Christoph recently fixed things so there was a non devm version
of these functions. Certainly we should not be making fake
platform_devices just to call devm.
There is also a struct device inside the cdev, maybe that could be
arrange to be devm compatible if it was *really* needed.
Will be fixed in v4.
diff --git a/include/Kbuild b/include/Kbuild
index ffba79483cc5..6ffb44a45957 100644
+++ 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.
No, it is right, it just shouldn't be in this makefile.
Jason
Will be fixed in v4.
Thanks for the review, the code is much simpler now.
