| Message ID | 20200123222112.26840-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: Release filp before global lock | expand |
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
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);
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(-)