On Tue, Nov 30, 2021 at 2:36 PM André Almeida <andrealmeid@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
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
>
