[v2] drm: Release filp before global lock
diff mbox series

Message ID 20200123222112.26840-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [v2] drm: Release filp before global lock
Related show

Commit Message

Chris Wilson Jan. 23, 2020, 10:21 p.m. UTC
The file is not part of the global drm resource and can be released
prior to take the global mutex to drop the open_count (and potentially
close) the drm device. As the global mutex is indeed global, not only
within the device but across devices, a slow file release mechanism can
bottleneck the entire system.

However, inside drm_close_helper() there are a number of dev->driver
callbacks that take the drm_device as the first parameter... Worryingly
some of those callbacks may be (implicitly) depending on the global
mutex.

v2: Drop the debug message for the open-count, it's included with the
drm_file_free() debug message -- and for good measure make that up as
reading outside of the mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
 drivers/gpu/drm/drm_file.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Thomas Hellström (VMware) Jan. 24, 2020, 12:29 p.m. UTC | #1
On 1/23/20 11:21 PM, Chris Wilson wrote:
> The file is not part of the global drm resource and can be released
> prior to take the global mutex to drop the open_count (and potentially
> close) the drm device. As the global mutex is indeed global, not only
> within the device but across devices, a slow file release mechanism can
> bottleneck the entire system.
>
> However, inside drm_close_helper() there are a number of dev->driver
> callbacks that take the drm_device as the first parameter... Worryingly
> some of those callbacks may be (implicitly) depending on the global
> mutex.

 From a quick audit, via, sis and vmwgfx are safe, so for those

Acked-by: Thomas Hellstrom <thellstrom@vmware.com>

Savage appears to be unsafe, due to unprotected access in the dma device 
member. Haven't audited i810 or potential other drivers affected. 
Perhaps it makes sense to enable lockfree filp release on a per-driver 
basis to begin with?

/Thomas

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 92d16724f949..777b450870a5 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@  void drm_file_free(struct drm_file *file)
 	DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
 		  task_pid_nr(current),
 		  (long)old_encode_dev(file->minor->kdev->devt),
-		  dev->open_count);
+		  READ_ONCE(dev->open_count));
 
 	if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
 	    dev->driver->preclose)
@@ -438,12 +438,10 @@  int drm_release(struct inode *inode, struct file *filp)
 	struct drm_minor *minor = file_priv->minor;
 	struct drm_device *dev = minor->dev;
 
-	mutex_lock(&drm_global_mutex);
-
-	DRM_DEBUG("open_count = %d\n", dev->open_count);
-
 	drm_close_helper(filp);
 
+	mutex_lock(&drm_global_mutex);
+
 	if (!--dev->open_count)
 		drm_lastclose(dev);