Re: [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem
From: Daniel Vetter
Date: Thu Aug 26 2021 - 05:55:24 EST
On Thu, Aug 26, 2021 at 10:01:17AM +0800, Desmond Cheong Zhi Xi wrote:
> drm_device.master_mutex currently protects the following:
> - drm_device.master
> - drm_file.master
> - drm_file.was_master
> - drm_file.is_master
> - drm_master.unique
> - drm_master.unique_len
> - drm_master.magic_map
>
> There is a clear separation between functions that read or change
> these attributes. Hence, convert master_mutex into a rwsem to enable
> concurrent readers.
>
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_auth.c | 35 ++++++++++++++++++-----------------
> drivers/gpu/drm/drm_debugfs.c | 4 ++--
> drivers/gpu/drm/drm_drv.c | 3 +--
> drivers/gpu/drm/drm_ioctl.c | 10 +++++-----
> include/drm/drm_auth.h | 6 +++---
> include/drm/drm_device.h | 10 ++++++----
> include/drm/drm_file.h | 12 ++++++------
> 7 files changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 60a6b21474b1..73ade0513ccb 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -64,7 +64,7 @@
> static bool drm_is_current_master_locked(struct drm_file *fpriv)
> {
> lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
> - lockdep_is_held(&fpriv->minor->dev->master_mutex));
> + lockdep_is_held(&fpriv->minor->dev->master_rwsem));
>
> return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
> }
> @@ -96,7 +96,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
> struct drm_auth *auth = data;
> int ret = 0;
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
> if (!file_priv->magic) {
> ret = idr_alloc(&file_priv->master->magic_map, file_priv,
> 1, 0, GFP_KERNEL);
> @@ -104,7 +104,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
> file_priv->magic = ret;
> }
> auth->magic = file_priv->magic;
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
>
> DRM_DEBUG("%u\n", auth->magic);
>
> @@ -119,13 +119,13 @@ int drm_authmagic(struct drm_device *dev, void *data,
>
> DRM_DEBUG("%u\n", auth->magic);
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
> file = idr_find(&file_priv->master->magic_map, auth->magic);
> if (file) {
> file->authenticated = 1;
> idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
> }
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
>
> return file ? 0 : -EINVAL;
> }
> @@ -167,7 +167,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> struct drm_master *old_master;
> struct drm_master *new_master;
>
> - lockdep_assert_held_once(&dev->master_mutex);
> + lockdep_assert_held_once(&dev->master_rwsem);
>
> WARN_ON(fpriv->is_master);
> old_master = fpriv->master;
> @@ -249,7 +249,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
> {
> int ret;
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
>
> ret = drm_master_check_perm(dev, file_priv);
> if (ret)
> @@ -281,7 +281,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>
> drm_set_master(dev, file_priv, false);
> out_unlock:
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
> return ret;
> }
>
> @@ -298,7 +298,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> {
> int ret;
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
>
> ret = drm_master_check_perm(dev, file_priv);
> if (ret)
> @@ -321,8 +321,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> }
>
> drm_drop_master(dev, file_priv);
> +
> out_unlock:
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
> return ret;
> }
>
> @@ -334,7 +335,7 @@ int drm_master_open(struct drm_file *file_priv)
> /* if there is no current master make this fd it, but do not create
> * any master object for render clients
> */
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
> if (!dev->master) {
> ret = drm_new_set_master(dev, file_priv);
> } else {
> @@ -342,7 +343,7 @@ int drm_master_open(struct drm_file *file_priv)
> file_priv->master = drm_master_get(dev->master);
> spin_unlock(&file_priv->master_lookup_lock);
> }
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
>
> return ret;
> }
> @@ -352,7 +353,7 @@ void drm_master_release(struct drm_file *file_priv)
> struct drm_device *dev = file_priv->minor->dev;
> struct drm_master *master;
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
> master = file_priv->master;
> if (file_priv->magic)
> idr_remove(&file_priv->master->magic_map, file_priv->magic);
> @@ -375,7 +376,7 @@ void drm_master_release(struct drm_file *file_priv)
> /* drop the master reference held by the file priv */
> if (file_priv->master)
> drm_master_put(&file_priv->master);
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
> }
>
> /**
> @@ -450,9 +451,9 @@ EXPORT_SYMBOL(drm_master_put);
> /* Used by drm_client and drm_fb_helper */
> bool drm_master_internal_acquire(struct drm_device *dev)
> {
> - mutex_lock(&dev->master_mutex);
> + down_read(&dev->master_rwsem);
> if (dev->master) {
> - mutex_unlock(&dev->master_mutex);
> + up_read(&dev->master_rwsem);
> return false;
> }
>
> @@ -463,6 +464,6 @@ EXPORT_SYMBOL(drm_master_internal_acquire);
> /* Used by drm_client and drm_fb_helper */
> void drm_master_internal_release(struct drm_device *dev)
> {
> - mutex_unlock(&dev->master_mutex);
> + up_read(&dev->master_rwsem);
> }
> EXPORT_SYMBOL(drm_master_internal_release);
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b0a826489488..b34c9c263188 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -55,7 +55,7 @@ static int drm_name_info(struct seq_file *m, void *data)
> struct drm_device *dev = minor->dev;
> struct drm_master *master;
>
> - mutex_lock(&dev->master_mutex);
> + down_read(&dev->master_rwsem);
> master = dev->master;
> seq_printf(m, "%s", dev->driver->name);
> if (dev->dev)
> @@ -65,7 +65,7 @@ static int drm_name_info(struct seq_file *m, void *data)
> if (dev->unique)
> seq_printf(m, " unique=%s", dev->unique);
> seq_printf(m, "\n");
> - mutex_unlock(&dev->master_mutex);
> + up_read(&dev->master_rwsem);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7a5097467ba5..4556bf42954c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -570,7 +570,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
> /* Prevent use-after-free in drm_managed_release when debugging is
> * enabled. Slightly awkward, but can't really be helped. */
> dev->dev = NULL;
> - mutex_destroy(&dev->master_mutex);
> mutex_destroy(&dev->clientlist_mutex);
> mutex_destroy(&dev->filelist_mutex);
> mutex_destroy(&dev->struct_mutex);
> @@ -611,7 +610,7 @@ static int drm_dev_init(struct drm_device *dev,
> mutex_init(&dev->struct_mutex);
> mutex_init(&dev->filelist_mutex);
> mutex_init(&dev->clientlist_mutex);
> - mutex_init(&dev->master_mutex);
> + init_rwsem(&dev->master_rwsem);
>
> ret = drmm_add_action(dev, drm_dev_init_release, NULL);
> if (ret)
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 26f3a9ede8fe..d25713b09b80 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -119,16 +119,16 @@ int drm_getunique(struct drm_device *dev, void *data,
> struct drm_unique *u = data;
> struct drm_master *master;
>
> - mutex_lock(&dev->master_mutex);
> + down_read(&dev->master_rwsem);
> master = file_priv->master;
> if (u->unique_len >= master->unique_len) {
> if (copy_to_user(u->unique, master->unique, master->unique_len)) {
> - mutex_unlock(&dev->master_mutex);
> + up_read(&dev->master_rwsem);
> return -EFAULT;
> }
> }
> u->unique_len = master->unique_len;
> - mutex_unlock(&dev->master_mutex);
> + up_read(&dev->master_rwsem);
>
> return 0;
> }
> @@ -385,7 +385,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
> struct drm_set_version *sv = data;
> int if_version, retcode = 0;
>
> - mutex_lock(&dev->master_mutex);
> + down_write(&dev->master_rwsem);
> if (sv->drm_di_major != -1) {
> if (sv->drm_di_major != DRM_IF_MAJOR ||
> sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
> @@ -420,7 +420,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
> sv->drm_di_minor = DRM_IF_MINOR;
> sv->drm_dd_major = dev->driver->major;
> sv->drm_dd_minor = dev->driver->minor;
> - mutex_unlock(&dev->master_mutex);
> + up_write(&dev->master_rwsem);
>
> return retcode;
> }
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index ba248ca8866f..f0a89e5fcaad 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -67,17 +67,17 @@ struct drm_master {
> struct drm_device *dev;
> /**
> * @unique: Unique identifier: e.g. busid. Protected by
> - * &drm_device.master_mutex.
> + * &drm_device.master_rwsem.
> */
> char *unique;
> /**
> * @unique_len: Length of unique field. Protected by
> - * &drm_device.master_mutex.
> + * &drm_device.master_rwsem.
> */
> int unique_len;
> /**
> * @magic_map: Map of used authentication tokens. Protected by
> - * &drm_device.master_mutex.
> + * &drm_device.master_rwsem.
> */
> struct idr magic_map;
> void *driver_priv;
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 604b1d1b2d72..142fb2f6e74d 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -107,7 +107,7 @@ struct drm_device {
> * @master:
> *
> * Currently active master for this device.
> - * Protected by &master_mutex
> + * Protected by &master_rwsem
> */
> struct drm_master *master;
>
> @@ -146,11 +146,13 @@ struct drm_device {
> struct mutex struct_mutex;
>
> /**
> - * @master_mutex:
> + * @master_rwsem:
> *
> - * Lock for &drm_minor.master and &drm_file.is_master
> + * Lock for &drm_device.master, &drm_file.was_master,
> + * &drm_file.is_master, &drm_file.master, &drm_master.unique,
> + * &drm_master.unique_len, and &drm_master.magic_map.
> */
> - struct mutex master_mutex;
> + struct rw_semaphore master_rwsem;
>
> /**
> * @open_count:
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index a3acb7ac3550..d12bb2ba7814 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -205,7 +205,7 @@ struct drm_file {
> * @was_master:
> *
> * This client has or had, master capability. Protected by struct
> - * &drm_device.master_mutex.
> + * &drm_device.master_rwsem.
> *
> * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the
> * client is or was master in the past.
> @@ -216,7 +216,7 @@ struct drm_file {
> * @is_master:
> *
> * This client is the creator of @master. Protected by struct
> - * &drm_device.master_mutex.
> + * &drm_device.master_rwsem.
> *
> * See also the :ref:`section on primary nodes and authentication
> * <drm_primary_node>`.
> @@ -227,19 +227,19 @@ struct drm_file {
> * @master:
> *
> * Master this node is currently associated with. Protected by struct
> - * &drm_device.master_mutex, and serialized by @master_lookup_lock.
> + * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
> *
> * Only relevant if drm_is_primary_client() returns true. Note that
> * this only matches &drm_device.master if the master is the currently
> * active one.
> *
> - * To update @master, both &drm_device.master_mutex and
> + * To update @master, both &drm_device.master_rwsem and
> * @master_lookup_lock need to be held, therefore holding either of
> * them is safe and enough for the read side.
> *
> * When dereferencing this pointer, either hold struct
> - * &drm_device.master_mutex for the duration of the pointer's use, or
> - * use drm_file_get_master() if struct &drm_device.master_mutex is not
> + * &drm_device.master_rwsem for the duration of the pointer's use, or
> + * use drm_file_get_master() if struct &drm_device.master_rwsem is not
> * currently held and there is no other need to hold it. This prevents
> * @master from being freed during use.
> *
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch