Re: [PATCH v4 2/2] mm/hmm/test: add self tests for HMM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/12/19 7:25 AM, Christoph Hellwig wrote:
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?

+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)
+			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.

Good point. I'll fix that.

+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))
+		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.

+static int dmirror_fops_open(struct inode *inode, struct file *filp)
+{
+	struct cdev *cdev = inode->i_cdev;
+	struct dmirror_device *mdevice;
+	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.

OK, I'll remove that test.

+	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..

+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.

+	filp->private_data = NULL;

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

OK, I'll clean that up.



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux