From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CDE0DC433FE for ; Wed, 9 Feb 2022 09:39:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233394AbiBIJi7 (ORCPT ); Wed, 9 Feb 2022 04:38:59 -0500 Received: from gmail-smtp-in.l.google.com ([23.128.96.19]:47122 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238860AbiBIJdq (ORCPT ); Wed, 9 Feb 2022 04:33:46 -0500 Received: from out199-12.us.a.mail.aliyun.com (out199-12.us.a.mail.aliyun.com [47.90.199.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 99731E011158 for ; Wed, 9 Feb 2022 01:33:39 -0800 (PST) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R101e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04426;MF=kanie@linux.alibaba.com;NM=1;PH=DS;RN=6;SR=0;TI=SMTPD_---0V4-ipji_1644398801; Received: from 30.178.82.5(mailfrom:kanie@linux.alibaba.com fp:SMTPD_---0V4-ipji_1644398801) by smtp.aliyun-inc.com(127.0.0.1); Wed, 09 Feb 2022 17:26:42 +0800 Message-ID: Date: Wed, 9 Feb 2022 17:26:41 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] uio: Replace mutex info_lock with percpu_ref to improve performance To: Christoph Hellwig Cc: gregkh@linuxfoundation.org, xiaoguang.wang@linux.alibaba.com, xlpang@linux.alibaba.com, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org References: <1644304760-11862-1-git-send-email-kanie@linux.alibaba.com> From: Guixin Liu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 >> Signed-off-by: Guixin Liu >> --- >> 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 >> #include >> #include >> +#include >> +#include >> >> #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 >> #include >> #include >> +#include >> >> 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---