* [PATCH 0/1] hidraw: Improve subsytem throughput @ 2021-11-30 13:29 André Almeida 2021-11-30 13:29 ` [PATCH 1/1] HID: hidraw: Replace hidraw device table mutex with a rwsem André Almeida 0 siblings, 1 reply; 3+ messages in thread From: André Almeida @ 2021-11-30 13:29 UTC (permalink / raw) To: Benjamin Tissoires, Jiri Kosina, linux-input, linux-kernel, kernel Cc: André Almeida Hi, In a discussion about SteamVR for Linux[0], users found out that modifying hidraw code would lead to more stable frame per seconds. This patch aims to fix a bottleneck at hidraw subsystem to increase the system throughput when using a lot of HIDs at same time. I created an artificial benchmark tool[1] to test this. It issues a lot of ioctls in parallel for 5 HID devices. While the original kernel took around 1570 nsecs to complete all the calls, after applying this patch it took around 412 nsecs, almost 4x faster. Thanks, André [0] https://github.com/ValveSoftware/SteamVR-for-Linux/issues/21 [1] https://gist.github.com/andrealmeid/b495999b8efce3c741e14a709a21e49b André Almeida (1): HID: hidraw: Replace hidraw device table mutex with a rwsem drivers/hid/hidraw.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/1] HID: hidraw: Replace hidraw device table mutex with a rwsem 2021-11-30 13:29 [PATCH 0/1] hidraw: Improve subsytem throughput André Almeida @ 2021-11-30 13:29 ` André Almeida 2021-12-14 12:31 ` Benjamin Tissoires 0 siblings, 1 reply; 3+ messages in thread From: André Almeida @ 2021-11-30 13:29 UTC (permalink / raw) To: Benjamin Tissoires, Jiri Kosina, linux-input, linux-kernel, kernel Cc: André Almeida Currently, the table that stores information about the connected hidraw devices has a mutex to prevent concurrent hidraw users to manipulate the hidraw table (e.g. delete an entry) while someone is trying to use the table (e.g. issuing an ioctl to the device), preventing the kernel to referencing a NULL pointer. However, since that every user that wants to access the table for both manipulating it and reading it content, this prevents concurrent access to the table for read-only operations for different or the same device (e.g. two hidraw ioctls can't happen at the same time, even if they are completely unrelated). This proves to be a bottleneck and gives performance issues when using multiple HID devices at same time, like VR kits where one can have two controllers, the headset and some tracking sensors. To improve the performance, replace the table mutex with a read-write semaphore, enabling multiple threads to issue parallel syscalls to multiple devices at the same time while protecting the table for concurrent modifications. Signed-off-by: André Almeida <andrealmeid@collabora.com> --- drivers/hid/hidraw.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c index 79faac87a06f..681614a8302a 100644 --- a/drivers/hid/hidraw.c +++ b/drivers/hid/hidraw.c @@ -34,7 +34,7 @@ static int hidraw_major; static struct cdev hidraw_cdev; static struct class *hidraw_class; static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES]; -static DEFINE_MUTEX(minors_lock); +static DECLARE_RWSEM(minors_rwsem); static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos) { @@ -107,7 +107,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, __u8 *buf; int ret = 0; - lockdep_assert_held(&minors_lock); + lockdep_assert_held(&minors_rwsem); if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { ret = -ENODEV; @@ -160,9 +160,9 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) { ssize_t ret; - mutex_lock(&minors_lock); + down_read(&minors_rwsem); ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT); - mutex_unlock(&minors_lock); + up_read(&minors_rwsem); return ret; } @@ -182,7 +182,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t int ret = 0, len; unsigned char report_number; - lockdep_assert_held(&minors_lock); + lockdep_assert_held(&minors_rwsem); if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { ret = -ENODEV; @@ -272,7 +272,7 @@ static int hidraw_open(struct inode *inode, struct file *file) goto out; } - mutex_lock(&minors_lock); + down_read(&minors_rwsem); if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { err = -ENODEV; goto out_unlock; @@ -301,7 +301,7 @@ static int hidraw_open(struct inode *inode, struct file *file) spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags); file->private_data = list; out_unlock: - mutex_unlock(&minors_lock); + up_read(&minors_rwsem); out: if (err < 0) kfree(list); @@ -347,7 +347,7 @@ static int hidraw_release(struct inode * inode, struct file * file) struct hidraw_list *list = file->private_data; unsigned long flags; - mutex_lock(&minors_lock); + down_write(&minors_rwsem); spin_lock_irqsave(&hidraw_table[minor]->list_lock, flags); list_del(&list->node); @@ -356,7 +356,7 @@ static int hidraw_release(struct inode * inode, struct file * file) drop_ref(hidraw_table[minor], 0); - mutex_unlock(&minors_lock); + up_write(&minors_rwsem); return 0; } @@ -369,7 +369,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd, struct hidraw *dev; void __user *user_arg = (void __user*) arg; - mutex_lock(&minors_lock); + down_read(&minors_rwsem); dev = hidraw_table[minor]; if (!dev || !dev->exist) { ret = -ENODEV; @@ -487,7 +487,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd, ret = -ENOTTY; } out: - mutex_unlock(&minors_lock); + up_read(&minors_rwsem); return ret; } @@ -546,7 +546,7 @@ int hidraw_connect(struct hid_device *hid) result = -EINVAL; - mutex_lock(&minors_lock); + down_write(&minors_rwsem); for (minor = 0; minor < HIDRAW_MAX_DEVICES; minor++) { if (hidraw_table[minor]) @@ -557,7 +557,7 @@ int hidraw_connect(struct hid_device *hid) } if (result) { - mutex_unlock(&minors_lock); + up_write(&minors_rwsem); kfree(dev); goto out; } @@ -567,7 +567,7 @@ int hidraw_connect(struct hid_device *hid) if (IS_ERR(dev->dev)) { hidraw_table[minor] = NULL; - mutex_unlock(&minors_lock); + up_write(&minors_rwsem); result = PTR_ERR(dev->dev); kfree(dev); goto out; @@ -583,7 +583,7 @@ int hidraw_connect(struct hid_device *hid) dev->exist = 1; hid->hidraw = dev; - mutex_unlock(&minors_lock); + up_write(&minors_rwsem); out: return result; @@ -594,11 +594,11 @@ void hidraw_disconnect(struct hid_device *hid) { struct hidraw *hidraw = hid->hidraw; - mutex_lock(&minors_lock); + down_write(&minors_rwsem); drop_ref(hidraw, 1); - mutex_unlock(&minors_lock); + up_write(&minors_rwsem); } EXPORT_SYMBOL_GPL(hidraw_disconnect); -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] HID: hidraw: Replace hidraw device table mutex with a rwsem 2021-11-30 13:29 ` [PATCH 1/1] HID: hidraw: Replace hidraw device table mutex with a rwsem André Almeida @ 2021-12-14 12:31 ` Benjamin Tissoires 0 siblings, 0 replies; 3+ messages in thread From: Benjamin Tissoires @ 2021-12-14 12:31 UTC (permalink / raw) To: André Almeida; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, kernel On Tue, Nov 30, 2021 at 2:36 PM André Almeida <andrealmeid@collabora.com> wrote: > > Currently, the table that stores information about the connected hidraw > devices has a mutex to prevent concurrent hidraw users to manipulate the > hidraw table (e.g. delete an entry) while someone is trying to use > the table (e.g. issuing an ioctl to the device), preventing the kernel > to referencing a NULL pointer. However, since that every user that wants > to access the table for both manipulating it and reading it content, > this prevents concurrent access to the table for read-only operations > for different or the same device (e.g. two hidraw ioctls can't happen at > the same time, even if they are completely unrelated). > > This proves to be a bottleneck and gives performance issues when using > multiple HID devices at same time, like VR kits where one can have two > controllers, the headset and some tracking sensors. > > To improve the performance, replace the table mutex with a read-write > semaphore, enabling multiple threads to issue parallel syscalls to > multiple devices at the same time while protecting the table for > concurrent modifications. > > Signed-off-by: André Almeida <andrealmeid@collabora.com> > --- Patch looks good to me. Applied to for-5.17/hidraw. Thanks! Cheers, Benjamin > drivers/hid/hidraw.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c > index 79faac87a06f..681614a8302a 100644 > --- a/drivers/hid/hidraw.c > +++ b/drivers/hid/hidraw.c > @@ -34,7 +34,7 @@ static int hidraw_major; > static struct cdev hidraw_cdev; > static struct class *hidraw_class; > static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES]; > -static DEFINE_MUTEX(minors_lock); > +static DECLARE_RWSEM(minors_rwsem); > > static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos) > { > @@ -107,7 +107,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, > __u8 *buf; > int ret = 0; > > - lockdep_assert_held(&minors_lock); > + lockdep_assert_held(&minors_rwsem); > > if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { > ret = -ENODEV; > @@ -160,9 +160,9 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, > static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos) > { > ssize_t ret; > - mutex_lock(&minors_lock); > + down_read(&minors_rwsem); > ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT); > - mutex_unlock(&minors_lock); > + up_read(&minors_rwsem); > return ret; > } > > @@ -182,7 +182,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t > int ret = 0, len; > unsigned char report_number; > > - lockdep_assert_held(&minors_lock); > + lockdep_assert_held(&minors_rwsem); > > if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { > ret = -ENODEV; > @@ -272,7 +272,7 @@ static int hidraw_open(struct inode *inode, struct file *file) > goto out; > } > > - mutex_lock(&minors_lock); > + down_read(&minors_rwsem); > if (!hidraw_table[minor] || !hidraw_table[minor]->exist) { > err = -ENODEV; > goto out_unlock; > @@ -301,7 +301,7 @@ static int hidraw_open(struct inode *inode, struct file *file) > spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags); > file->private_data = list; > out_unlock: > - mutex_unlock(&minors_lock); > + up_read(&minors_rwsem); > out: > if (err < 0) > kfree(list); > @@ -347,7 +347,7 @@ static int hidraw_release(struct inode * inode, struct file * file) > struct hidraw_list *list = file->private_data; > unsigned long flags; > > - mutex_lock(&minors_lock); > + down_write(&minors_rwsem); > > spin_lock_irqsave(&hidraw_table[minor]->list_lock, flags); > list_del(&list->node); > @@ -356,7 +356,7 @@ static int hidraw_release(struct inode * inode, struct file * file) > > drop_ref(hidraw_table[minor], 0); > > - mutex_unlock(&minors_lock); > + up_write(&minors_rwsem); > return 0; > } > > @@ -369,7 +369,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd, > struct hidraw *dev; > void __user *user_arg = (void __user*) arg; > > - mutex_lock(&minors_lock); > + down_read(&minors_rwsem); > dev = hidraw_table[minor]; > if (!dev || !dev->exist) { > ret = -ENODEV; > @@ -487,7 +487,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd, > ret = -ENOTTY; > } > out: > - mutex_unlock(&minors_lock); > + up_read(&minors_rwsem); > return ret; > } > > @@ -546,7 +546,7 @@ int hidraw_connect(struct hid_device *hid) > > result = -EINVAL; > > - mutex_lock(&minors_lock); > + down_write(&minors_rwsem); > > for (minor = 0; minor < HIDRAW_MAX_DEVICES; minor++) { > if (hidraw_table[minor]) > @@ -557,7 +557,7 @@ int hidraw_connect(struct hid_device *hid) > } > > if (result) { > - mutex_unlock(&minors_lock); > + up_write(&minors_rwsem); > kfree(dev); > goto out; > } > @@ -567,7 +567,7 @@ int hidraw_connect(struct hid_device *hid) > > if (IS_ERR(dev->dev)) { > hidraw_table[minor] = NULL; > - mutex_unlock(&minors_lock); > + up_write(&minors_rwsem); > result = PTR_ERR(dev->dev); > kfree(dev); > goto out; > @@ -583,7 +583,7 @@ int hidraw_connect(struct hid_device *hid) > dev->exist = 1; > hid->hidraw = dev; > > - mutex_unlock(&minors_lock); > + up_write(&minors_rwsem); > out: > return result; > > @@ -594,11 +594,11 @@ void hidraw_disconnect(struct hid_device *hid) > { > struct hidraw *hidraw = hid->hidraw; > > - mutex_lock(&minors_lock); > + down_write(&minors_rwsem); > > drop_ref(hidraw, 1); > > - mutex_unlock(&minors_lock); > + up_write(&minors_rwsem); > } > EXPORT_SYMBOL_GPL(hidraw_disconnect); > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-14 12:31 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-30 13:29 [PATCH 0/1] hidraw: Improve subsytem throughput André Almeida 2021-11-30 13:29 ` [PATCH 1/1] HID: hidraw: Replace hidraw device table mutex with a rwsem André Almeida 2021-12-14 12:31 ` Benjamin Tissoires
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.