| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e9f8c595-c133-54f4-1167-31f4a8a6e52e@linux.alibaba.com>
Date: Wed, 9 Feb 2022 17:26:41 +0800
From: Guixin Liu <kanie@...ux.alibaba.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: gregkh@...uxfoundation.org, xiaoguang.wang@...ux.alibaba.com,
xlpang@...ux.alibaba.com, linux-kernel@...r.kernel.org,
iommu@...ts.linux-foundation.org
Subject: Re: [PATCH] uio: Replace mutex info_lock with percpu_ref to improve
performance
在 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@...ux.alibaba.com>
>> Signed-off-by: Guixin Liu <kanie@...ux.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---
Powered by blists - more mailing lists