Linux IOMMU Development
 help / color / mirror / Atom feed
From: Guixin Liu <kanie@linux.alibaba.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: gregkh@linuxfoundation.org, iommu@lists.linux-foundation.org,
	xlpang@linux.alibaba.com, linux-kernel@vger.kernel.org,
	xiaoguang.wang@linux.alibaba.com
Subject: Re: [PATCH] uio: Replace mutex info_lock with percpu_ref to improve performance
Date: Wed, 9 Feb 2022 17:26:41 +0800	[thread overview]
Message-ID: <e9f8c595-c133-54f4-1167-31f4a8a6e52e@linux.alibaba.com> (raw)
In-Reply-To: <YgNv9folAsgtGl5Z@infradead.org>


在 2022/2/9 15:40, Christoph Hellwig 写道:
> On Tue, Feb 08, 2022 at 03:19:20PM +0800, Guixin Liu wrote:
>> This patch includes a modification to repace mutex info_lock with
>> percpu_ref, in order to improve uio performance.
> What performance critical use case do you have for uio?  Everyone really
> should be using vfio these days due to the large amount of shortcomings
> in the uio interface.
We use uio because the tcmu and tcmu-runner use uio，and in fact tcmu is 
not a real hardware.
>
>> Reviewed-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/uio/uio.c          | 95 ++++++++++++++++++++++++++++++++++------------
>>   include/linux/uio_driver.h |  5 ++-
>>   2 files changed, 75 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index 43afbb7..0cc0655 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -24,6 +24,8 @@
>>   #include <linux/kobject.h>
>>   #include <linux/cdev.h>
>>   #include <linux/uio_driver.h>
>> +#include <linux/completion.h>
>> +#include <linux/percpu-refcount.h>
>>   
>>   #define UIO_MAX_DEVICES		(1U << MINORBITS)
>>   
>> @@ -218,7 +220,9 @@ static ssize_t name_show(struct device *dev,
>>   	struct uio_device *idev = dev_get_drvdata(dev);
>>   	int ret;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info) {
>>   		ret = -EINVAL;
>>   		dev_err(dev, "the device has been unregistered\n");
>> @@ -228,7 +232,7 @@ static ssize_t name_show(struct device *dev,
>>   	ret = sprintf(buf, "%s\n", idev->info->name);
>>   
>>   out:
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   	return ret;
>>   }
>>   static DEVICE_ATTR_RO(name);
>> @@ -239,7 +243,9 @@ static ssize_t version_show(struct device *dev,
>>   	struct uio_device *idev = dev_get_drvdata(dev);
>>   	int ret;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info) {
>>   		ret = -EINVAL;
>>   		dev_err(dev, "the device has been unregistered\n");
>> @@ -249,7 +255,7 @@ static ssize_t version_show(struct device *dev,
>>   	ret = sprintf(buf, "%s\n", idev->info->version);
>>   
>>   out:
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   	return ret;
>>   }
>>   static DEVICE_ATTR_RO(version);
>> @@ -489,16 +495,20 @@ static int uio_open(struct inode *inode, struct file *filep)
>>   	listener->event_count = atomic_read(&idev->event);
>>   	filep->private_data = listener;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref)) {
>> +		ret = -EINVAL;
>> +		goto err_infoopen;
>> +	}
>> +
>>   	if (!idev->info) {
>> -		mutex_unlock(&idev->info_lock);
>> +		percpu_ref_put(&idev->info_ref);
>>   		ret = -EINVAL;
>>   		goto err_infoopen;
>>   	}
>>   
>>   	if (idev->info->open)
>>   		ret = idev->info->open(idev->info, inode);
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   	if (ret)
>>   		goto err_infoopen;
>>   
>> @@ -531,10 +541,12 @@ static int uio_release(struct inode *inode, struct file *filep)
>>   	struct uio_listener *listener = filep->private_data;
>>   	struct uio_device *idev = listener->dev;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (idev->info && idev->info->release)
>>   		ret = idev->info->release(idev->info, inode);
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   
>>   	module_put(idev->owner);
>>   	kfree(listener);
>> @@ -548,10 +560,12 @@ static __poll_t uio_poll(struct file *filep, poll_table *wait)
>>   	struct uio_device *idev = listener->dev;
>>   	__poll_t ret = 0;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info || !idev->info->irq)
>>   		ret = -EIO;
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   
>>   	if (ret)
>>   		return ret;
>> @@ -577,13 +591,17 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   	add_wait_queue(&idev->wait, &wait);
>>   
>>   	do {
>> -		mutex_lock(&idev->info_lock);
>> +		if (!percpu_ref_tryget_live(&idev->info_ref)) {
>> +			retval = -EINVAL;
>> +			break;
>> +		}
>> +
>>   		if (!idev->info || !idev->info->irq) {
>>   			retval = -EIO;
>> -			mutex_unlock(&idev->info_lock);
>> +			percpu_ref_put(&idev->info_ref);
>>   			break;
>>   		}
>> -		mutex_unlock(&idev->info_lock);
>> +		percpu_ref_put(&idev->info_ref);
>>   
>>   		set_current_state(TASK_INTERRUPTIBLE);
>>   
>> @@ -631,7 +649,9 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
>>   	if (copy_from_user(&irq_on, buf, count))
>>   		return -EFAULT;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info) {
>>   		retval = -EINVAL;
>>   		goto out;
>> @@ -650,7 +670,7 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
>>   	retval = idev->info->irqcontrol(idev->info, irq_on);
>>   
>>   out:
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   	return retval ? retval : sizeof(s32);
>>   }
>>   
>> @@ -675,7 +695,9 @@ static vm_fault_t uio_vma_fault(struct vm_fault *vmf)
>>   	vm_fault_t ret = 0;
>>   	int mi;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info) {
>>   		ret = VM_FAULT_SIGBUS;
>>   		goto out;
>> @@ -702,8 +724,7 @@ static vm_fault_t uio_vma_fault(struct vm_fault *vmf)
>>   	vmf->page = page;
>>   
>>   out:
>> -	mutex_unlock(&idev->info_lock);
>> -
>> +	percpu_ref_put(&idev->info_ref);
>>   	return ret;
>>   }
>>   
>> @@ -772,7 +793,9 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>   
>>   	vma->vm_private_data = idev;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	if (!percpu_ref_tryget_live(&idev->info_ref))
>> +		return -EINVAL;
>> +
>>   	if (!idev->info) {
>>   		ret = -EINVAL;
>>   		goto out;
>> @@ -811,7 +834,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>   	}
>>   
>>    out:
>> -	mutex_unlock(&idev->info_lock);
>> +	percpu_ref_put(&idev->info_ref);
>>   	return ret;
>>   }
>>   
>> @@ -907,6 +930,13 @@ static void uio_device_release(struct device *dev)
>>   	kfree(idev);
>>   }
>>   
>> +static void uio_info_free(struct percpu_ref *ref)
>> +{
>> +	struct uio_device *idev = container_of(ref, struct uio_device, info_ref);
>> +
>> +	complete(&idev->free_done);
>> +}
>> +
>>   /**
>>    * __uio_register_device - register a new userspace IO device
>>    * @owner:	module that creates the new device
>> @@ -937,10 +967,17 @@ int __uio_register_device(struct module *owner,
>>   
>>   	idev->owner = owner;
>>   	idev->info = info;
>> -	mutex_init(&idev->info_lock);
>>   	init_waitqueue_head(&idev->wait);
>>   	atomic_set(&idev->event, 0);
>>   
>> +	ret = percpu_ref_init(&idev->info_ref, uio_info_free, 0, GFP_KERNEL);
>> +	if (ret) {
>> +		 pr_err("percpu_ref init failed!\n");
>> +		 return ret;
>> +	}
>> +	init_completion(&idev->confirm_done);
>> +	init_completion(&idev->free_done);
>> +
>>   	ret = uio_get_minor(idev);
>>   	if (ret) {
>>   		kfree(idev);
>> @@ -1036,6 +1073,13 @@ int __devm_uio_register_device(struct module *owner,
>>   }
>>   EXPORT_SYMBOL_GPL(__devm_uio_register_device);
>>   
>> +static void uio_confirm_info(struct percpu_ref *ref)
>> +{
>> +	struct uio_device *idev = container_of(ref, struct uio_device, info_ref);
>> +
>> +	complete(&idev->confirm_done);
>> +}
>> +
>>   /**
>>    * uio_unregister_device - unregister a industrial IO device
>>    * @info:	UIO device capabilities
>> @@ -1052,14 +1096,17 @@ void uio_unregister_device(struct uio_info *info)
>>   	idev = info->uio_dev;
>>   	minor = idev->minor;
>>   
>> -	mutex_lock(&idev->info_lock);
>> +	percpu_ref_kill_and_confirm(&idev->info_ref, uio_confirm_info);
>> +	wait_for_completion(&idev->confirm_done);
>> +	wait_for_completion(&idev->free_done);
>> +
>> +	/* now, we can set info to NULL */
>>   	uio_dev_del_attributes(idev);
>>   
>>   	if (info->irq && info->irq != UIO_IRQ_CUSTOM)
>>   		free_irq(info->irq, idev);
>>   
>>   	idev->info = NULL;
>> -	mutex_unlock(&idev->info_lock);
>>   
>>   	wake_up_interruptible(&idev->wait);
>>   	kill_fasync(&idev->async_queue, SIGIO, POLL_HUP);
>> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
>> index 47c5962..6d3d87f 100644
>> --- a/include/linux/uio_driver.h
>> +++ b/include/linux/uio_driver.h
>> @@ -16,6 +16,7 @@
>>   #include <linux/device.h>
>>   #include <linux/fs.h>
>>   #include <linux/interrupt.h>
>> +#include <linux/percpu-refcount.h>
>>   
>>   struct module;
>>   struct uio_map;
>> @@ -74,9 +75,11 @@ struct uio_device {
>>   	struct fasync_struct    *async_queue;
>>   	wait_queue_head_t       wait;
>>   	struct uio_info         *info;
>> -	struct mutex		info_lock;
>>   	struct kobject          *map_dir;
>>   	struct kobject          *portio_dir;
>> +	struct percpu_ref       info_ref;
>> +	struct completion       confirm_done;
>> +	struct completion       free_done;
>>   };
>>   
>>   /**
>> -- 
>> 1.8.3.1
>>
> ---end quoted text---
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

      reply	other threads:[~2022-02-09 15:22 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1644304760-11862-1-git-send-email-kanie@linux.alibaba.com>
2022-02-09  7:40 ` [PATCH] uio: Replace mutex info_lock with percpu_ref to improve performance Christoph Hellwig
2022-02-09  9:26   ` Guixin Liu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e9f8c595-c133-54f4-1167-31f4a8a6e52e@linux.alibaba.com \
    --to=kanie@linux.alibaba.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=xiaoguang.wang@linux.alibaba.com \
    --cc=xlpang@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox