This patch includes a modification to repace mutex info_lock with
percpu_ref, in order to improve uio performance.
Reviewed-by: Xiaoguang Wang <[email protected]>
Signed-off-by: Guixin Liu <[email protected]>
---
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
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.
How does it improve performance? What benchmark are you using and what
are the results?
These changes are quite complex, you need to better describe these in
order to be able to have them accepted.
thanks,
greg k-h
Hi Guixin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v5.17-rc3 next-20220208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guixin-Liu/uio-Replace-mutex-info_lock-with-percpu_ref-to-improve-performance/20220208-152119
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git b86f32951d173b43d1db8de883473fc53dc3c772
config: parisc-randconfig-s032-20220208 (https://download.01.org/0day-ci/archive/20220208/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/271c7d04f21a7c880e9dfb056eca51835576833d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guixin-Liu/uio-Replace-mutex-info_lock-with-percpu_ref-to-improve-performance/20220208-152119
git checkout 271c7d04f21a7c880e9dfb056eca51835576833d
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc SHELL=/bin/bash drivers/uio/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/uio/uio.c:564:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __poll_t @@ got int @@
drivers/uio/uio.c:564:24: sparse: expected restricted __poll_t
drivers/uio/uio.c:564:24: sparse: got int
drivers/uio/uio.c:567:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __poll_t [usertype] ret @@ got int @@
drivers/uio/uio.c:567:21: sparse: expected restricted __poll_t [usertype] ret
drivers/uio/uio.c:567:21: sparse: got int
>> drivers/uio/uio.c:699:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted vm_fault_t @@ got int @@
drivers/uio/uio.c:699:24: sparse: expected restricted vm_fault_t
drivers/uio/uio.c:699:24: sparse: got int
vim +564 drivers/uio/uio.c
556
557 static __poll_t uio_poll(struct file *filep, poll_table *wait)
558 {
559 struct uio_listener *listener = filep->private_data;
560 struct uio_device *idev = listener->dev;
561 __poll_t ret = 0;
562
563 if (!percpu_ref_tryget_live(&idev->info_ref))
> 564 return -EINVAL;
565
566 if (!idev->info || !idev->info->irq)
567 ret = -EIO;
568 percpu_ref_put(&idev->info_ref);
569
570 if (ret)
571 return ret;
572
573 poll_wait(filep, &idev->wait, wait);
574 if (listener->event_count != atomic_read(&idev->event))
575 return EPOLLIN | EPOLLRDNORM;
576 return 0;
577 }
578
579 static ssize_t uio_read(struct file *filep, char __user *buf,
580 size_t count, loff_t *ppos)
581 {
582 struct uio_listener *listener = filep->private_data;
583 struct uio_device *idev = listener->dev;
584 DECLARE_WAITQUEUE(wait, current);
585 ssize_t retval = 0;
586 s32 event_count;
587
588 if (count != sizeof(s32))
589 return -EINVAL;
590
591 add_wait_queue(&idev->wait, &wait);
592
593 do {
594 if (!percpu_ref_tryget_live(&idev->info_ref)) {
595 retval = -EINVAL;
596 break;
597 }
598
599 if (!idev->info || !idev->info->irq) {
600 retval = -EIO;
601 percpu_ref_put(&idev->info_ref);
602 break;
603 }
604 percpu_ref_put(&idev->info_ref);
605
606 set_current_state(TASK_INTERRUPTIBLE);
607
608 event_count = atomic_read(&idev->event);
609 if (event_count != listener->event_count) {
610 __set_current_state(TASK_RUNNING);
611 if (copy_to_user(buf, &event_count, count))
612 retval = -EFAULT;
613 else {
614 listener->event_count = event_count;
615 retval = count;
616 }
617 break;
618 }
619
620 if (filep->f_flags & O_NONBLOCK) {
621 retval = -EAGAIN;
622 break;
623 }
624
625 if (signal_pending(current)) {
626 retval = -ERESTARTSYS;
627 break;
628 }
629 schedule();
630 } while (1);
631
632 __set_current_state(TASK_RUNNING);
633 remove_wait_queue(&idev->wait, &wait);
634
635 return retval;
636 }
637
638 static ssize_t uio_write(struct file *filep, const char __user *buf,
639 size_t count, loff_t *ppos)
640 {
641 struct uio_listener *listener = filep->private_data;
642 struct uio_device *idev = listener->dev;
643 ssize_t retval;
644 s32 irq_on;
645
646 if (count != sizeof(s32))
647 return -EINVAL;
648
649 if (copy_from_user(&irq_on, buf, count))
650 return -EFAULT;
651
652 if (!percpu_ref_tryget_live(&idev->info_ref))
653 return -EINVAL;
654
655 if (!idev->info) {
656 retval = -EINVAL;
657 goto out;
658 }
659
660 if (!idev->info->irq) {
661 retval = -EIO;
662 goto out;
663 }
664
665 if (!idev->info->irqcontrol) {
666 retval = -ENOSYS;
667 goto out;
668 }
669
670 retval = idev->info->irqcontrol(idev->info, irq_on);
671
672 out:
673 percpu_ref_put(&idev->info_ref);
674 return retval ? retval : sizeof(s32);
675 }
676
677 static int uio_find_mem_index(struct vm_area_struct *vma)
678 {
679 struct uio_device *idev = vma->vm_private_data;
680
681 if (vma->vm_pgoff < MAX_UIO_MAPS) {
682 if (idev->info->mem[vma->vm_pgoff].size == 0)
683 return -1;
684 return (int)vma->vm_pgoff;
685 }
686 return -1;
687 }
688
689 static vm_fault_t uio_vma_fault(struct vm_fault *vmf)
690 {
691 struct uio_device *idev = vmf->vma->vm_private_data;
692 struct page *page;
693 unsigned long offset;
694 void *addr;
695 vm_fault_t ret = 0;
696 int mi;
697
698 if (!percpu_ref_tryget_live(&idev->info_ref))
> 699 return -EINVAL;
700
701 if (!idev->info) {
702 ret = VM_FAULT_SIGBUS;
703 goto out;
704 }
705
706 mi = uio_find_mem_index(vmf->vma);
707 if (mi < 0) {
708 ret = VM_FAULT_SIGBUS;
709 goto out;
710 }
711
712 /*
713 * We need to subtract mi because userspace uses offset = N*PAGE_SIZE
714 * to use mem[N].
715 */
716 offset = (vmf->pgoff - mi) << PAGE_SHIFT;
717
718 addr = (void *)(unsigned long)idev->info->mem[mi].addr + offset;
719 if (idev->info->mem[mi].memtype == UIO_MEM_LOGICAL)
720 page = virt_to_page(addr);
721 else
722 page = vmalloc_to_page(addr);
723 get_page(page);
724 vmf->page = page;
725
726 out:
727 percpu_ref_put(&idev->info_ref);
728 return ret;
729 }
730
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
在 2022/2/8 15:25, Greg KH 写道:
> 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.
> How does it improve performance? What benchmark are you using and what
> are the results?
>
> These changes are quite complex, you need to better describe these in
> order to be able to have them accepted.
>
> thanks,
>
> greg k-h
okay, I will supplement the performance-test result later.
Guixin Liu
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 <[email protected]>
> Signed-off-by: Guixin Liu <[email protected]>
> ---
> 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---
在 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 <[email protected]>
>> Signed-off-by: Guixin Liu <[email protected]>
>> ---
>> 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---