| 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: <YgNv9folAsgtGl5Z@infradead.org>
Date: Tue, 8 Feb 2022 23:40:37 -0800
From: Christoph Hellwig <hch@...radead.org>
To: Guixin Liu <kanie@...ux.alibaba.com>
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
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.
>
> 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