Re: [PATCH] HMM selftests changes
From: Mika Penttilä
Date: Thu Mar 10 2022 - 00:34:07 EST
On 10.3.2022 5.44, John Hubbard wrote:
On 3/8/22 5:10 AM, Vlastimil Babka wrote:
+CC relevant folks
On 3/8/22 02:57, mpenttil@xxxxxxxxxx wrote:
From: Mika Penttilä <mpenttil@xxxxxxxxxx>
Hi Mika,
This looks like a very nice cleanup and simplification, delighted to see
it!
Thanks!
In fact, I like it so much that I've got a bunch of fussy nitpicks I'm
hoping you'll consider applying, below. :)
Also, I'm assuming you've tested it a little bit? (My test rig is boxed
up for another day or two, don't ask...haha.)
Yes it passed the tests with the changes.
HMM selftests use a in-kernel pseudo device to emulate device private memory.
For now, the pseudo device registers a major device range for two pseudo device instances.
User space has a script that goes figures out from /proc/devices the assigned major
and mknods the device nodes.
Change this use to a more standard device framework, like misc device,
which makes the device node names to appear right under any decent user space.
This also makes it possible for udev- like processing if wanted,
and the /proc/devices parsing is not needed any more.
How about this for the subject line and body? I've made the subject line
match what other commits look like in this area, and I've clarified the
wording in the body:
mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev
HMM selftests use an in-kernel pseudo device to emulate device private
memory. The pseudo device registers a major device range for two pseudo
device instances. User space has a script that reads /proc/devices in
order to find the assigned major number, and sends that to mknod(1),
once for each node.
This duplicates a fair amount of boilerplate that misc device can do
instead.
Change this to use misc device, which makes the device node names appear
for us. This also enables udev-like processing if desired.
Delete the /proc/devices parsing from the user-space test script, now
that it is unnecessary.
Signed-off-by: Mika Penttilä <mpenttil@xxxxxxxxxx>
---
lib/test_hmm.c | 46 +++++++++++++++-----------
tools/testing/selftests/vm/test_hmm.sh | 6 ----
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 767538089a62..76f6129e1f1f 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -10,7 +10,6 @@
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/kernel.h>
-#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/rwsem.h>
@@ -25,18 +24,25 @@
#include <linux/swapops.h>
#include <linux/sched/mm.h>
#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
#include <linux/rmap.h>
#include "test_hmm_uapi.h"
-#define DMIRROR_NDEVICES 2
#define DMIRROR_RANGE_FAULT_TIMEOUT 1000
#define DEVMEM_CHUNK_SIZE (256 * 1024 * 1024U)
#define DEVMEM_CHUNKS_RESERVE 16
+
Extra newline, please delete.
yep
+static const char *dmirror_device_names[] = {
+ "hmm_dmirror0",
+ "hmm_dmirror1"
+};
+
+#define DMIRROR_NDEVICES ARRAY_SIZE(dmirror_device_names)
+
static const struct dev_pagemap_ops dmirror_devmem_ops;
static const struct mmu_interval_notifier_ops dmirror_min_ops;
-static dev_t dmirror_dev;
struct dmirror_device;
@@ -82,7 +88,7 @@ struct dmirror_chunk {
* Per device data.
*/
struct dmirror_device {
- struct cdev cdevice;
+ struct miscdevice miscdevice;
struct hmm_devmem *devmem;
unsigned int devmem_capacity;
@@ -118,16 +124,20 @@ static void dmirror_bounce_fini(struct dmirror_bounce *bounce)
static int dmirror_fops_open(struct inode *inode, struct file *filp)
{
- struct cdev *cdev = inode->i_cdev;
+
+ struct dmirror_device *dd = container_of(filp->private_data,
+ struct dmirror_device, miscdevice
+ );
Let's delete that whole chunk, because the variable is unnecessary, and
is adding to the line count and mental load for no reason.
Agreed.
Look how much cleaner this it gets after applying this incremental diff
on top of your patch:
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 76f6129e1f1f..3ec8ca8059cd 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -125,19 +125,16 @@ static void dmirror_bounce_fini(struct dmirror_bounce *bounce)
static int dmirror_fops_open(struct inode *inode, struct file *filp)
{
- struct dmirror_device *dd = container_of(filp->private_data,
- struct dmirror_device, miscdevice
- );
struct dmirror *dmirror;
int ret;
-
/* Mirror this process address space */
dmirror = kzalloc(sizeof(*dmirror), GFP_KERNEL);
if (dmirror == NULL)
return -ENOMEM;
- dmirror->mdevice = dd;
+ dmirror->mdevice = container_of(filp->private_data,struct dmirror_device,
+ miscdevice);
mutex_init(&dmirror->mutex);
xa_init(&dmirror->pt);
struct dmirror *dmirror;
int ret;
+
/* Mirror this process address space */
dmirror = kzalloc(sizeof(*dmirror), GFP_KERNEL);
if (dmirror == NULL)
return -ENOMEM;
- dmirror->mdevice = container_of(cdev, struct dmirror_device, cdevice);
+ dmirror->mdevice = dd;
mutex_init(&dmirror->mutex);
xa_init(&dmirror->pt);
@@ -1216,16 +1226,18 @@ static const struct dev_pagemap_ops dmirror_devmem_ops = {
static int dmirror_device_init(struct dmirror_device *mdevice, int id)
{
- dev_t dev;
+
int ret;
- dev = MKDEV(MAJOR(dmirror_dev), id);
mutex_init(&mdevice->devmem_lock);
spin_lock_init(&mdevice->lock);
- cdev_init(&mdevice->cdevice, &dmirror_fops);
- mdevice->cdevice.owner = THIS_MODULE;
- ret = cdev_add(&mdevice->cdevice, dev, 1);
+ mdevice->miscdevice.minor = MISC_DYNAMIC_MINOR;
+ mdevice->miscdevice.name = dmirror_device_names[id];
+ mdevice->miscdevice.fops = &dmirror_fops;
+
+ ret = misc_register(&mdevice->miscdevice);
+
if (ret)
return ret;
@@ -1252,7 +1264,7 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
kfree(mdevice->devmem_chunks);
}
- cdev_del(&mdevice->cdevice);
+ misc_deregister(&mdevice->miscdevice);
}
static int __init hmm_dmirror_init(void)
@@ -1260,11 +1272,6 @@ static int __init hmm_dmirror_init(void)
int ret;
int id;
- ret = alloc_chrdev_region(&dmirror_dev, 0, DMIRROR_NDEVICES,
- "HMM_DMIRROR");
- if (ret)
- goto err_unreg;
-
for (id = 0; id < DMIRROR_NDEVICES; id++) {
ret = dmirror_device_init(dmirror_devices + id, id);
if (ret)
@@ -1277,8 +1284,7 @@ static int __init hmm_dmirror_init(void)
err_chrdev:
while (--id >= 0)
dmirror_device_remove(dmirror_devices + id);
- unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
-err_unreg:
+
return ret;
}
@@ -1288,7 +1294,7 @@ static void __exit hmm_dmirror_exit(void)
for (id = 0; id < DMIRROR_NDEVICES; id++)
dmirror_device_remove(dmirror_devices + id);
- unregister_chrdev_region(dmirror_dev, DMIRROR_NDEVICES);
+
Another extra newline, please delete it.
yes
}
module_init(hmm_dmirror_init);
diff --git a/tools/testing/selftests/vm/test_hmm.sh b/tools/testing/selftests/vm/test_hmm.sh
index 0647b525a625..69f5889f8575 100755
--- a/tools/testing/selftests/vm/test_hmm.sh
+++ b/tools/testing/selftests/vm/test_hmm.sh
@@ -41,17 +41,11 @@ check_test_requirements()
load_driver()
{
modprobe $DRIVER > /dev/null 2>&1
- if [ $? == 0 ]; then
- major=$(awk "\$2==\"HMM_DMIRROR\" {print \$1}" /proc/devices)
- mknod /dev/hmm_dmirror0 c $major 0
- mknod /dev/hmm_dmirror1 c $major 1
- fi
}
unload_driver()
{
modprobe -r $DRIVER > /dev/null 2>&1
- rm -f /dev/hmm_dmirror?
Nice!
}
run_smoke()
thanks,
Ok will make the changes and repost,
thanks!
--Mika