Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Remove superfluous locking around userfault_list

[Joonas Lahtinen]

On pe, 2016-10-14 at 12:05 +0100, Chris Wilson wrote:
> Now that we have reduced the access to the list to either (a) under the
> struct_mutex whilst holding the RPM wakeref (so that concurrent writers to
> the list are serialised by struct_mutex) and (b) under the atomic
> runtime suspend (which cannot run concurrently with any other accessor due
> to the atomic nature of the runtime suspend) we can remove the extra
> locking around the list itself.
> 
> Signed-off-by: Chris Wilson 

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/5] drm/i915: Remove superfluous locking around userfault_list

[Chris Wilson]

Now that we have reduced the access to the list to either (a) under the
struct_mutex whilst holding the RPM wakeref (so that concurrent writers to
the list are serialised by struct_mutex) and (b) under the atomic
runtime suspend (which cannot run concurrently with any other accessor due
to the atomic nature of the runtime suspend) we can remove the extra
locking around the list itself.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ---
 drivers/gpu/drm/i915/i915_gem.c | 33 -
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 593bf3c2063d..b3c00cc6ab32 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1360,9 +1360,6 @@ struct i915_gem_mm {
 */
struct list_head unbound_list;
 
-   /** Protects access to the userfault_list */
-   spinlock_t userfault_lock;
-
/** List of all objects in gtt_space, currently mmaped by userspace.
 * All objects within this list must also be on bound_list.
 */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d1ab59fb7a02..8819fafbcc7a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1844,10 +1844,8 @@ int i915_gem_fault(struct vm_area_struct *area, struct 
vm_fault *vmf)
 
/* Mark as being mmapped into userspace for later revocation */
assert_rpm_wakelock_held(dev_priv);
-   spin_lock(_priv->mm.userfault_lock);
if (list_empty(>userfault_link))
list_add(>userfault_link, _priv->mm.userfault_list);
-   spin_unlock(_priv->mm.userfault_lock);
 
/* Finally, remap it using the new GTT offset */
ret = remap_io_mapping(area,
@@ -1924,7 +1922,6 @@ void
 i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 {
struct drm_i915_private *i915 = to_i915(obj->base.dev);
-   bool zap = false;
 
/* Serialisation between user GTT access and our code depends upon
 * revoking the CPU's PTE whilst the mutex is held. The next user
@@ -1937,15 +1934,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
lockdep_assert_held(>drm.struct_mutex);
intel_runtime_pm_get(i915);
 
-   spin_lock(>mm.userfault_lock);
-   if (!list_empty(>userfault_link)) {
-   list_del_init(>userfault_link);
-   zap = true;
-   }
-   spin_unlock(>mm.userfault_lock);
-   if (!zap)
+   if (list_empty(>userfault_link))
goto out;
 
+   list_del_init(>userfault_link);
drm_vma_node_unmap(>base.vma_node,
   obj->base.dev->anon_inode->i_mapping);
 
@@ -1965,21 +1957,21 @@ out:
 void
 i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
 {
-   struct drm_i915_gem_object *obj;
+   struct drm_i915_gem_object *obj, *on;
 
-   spin_lock(_priv->mm.userfault_lock);
-   while ((obj = list_first_entry_or_null(_priv->mm.userfault_list,
-  struct drm_i915_gem_object,
-  userfault_link))) {
-   list_del_init(>userfault_link);
-   spin_unlock(_priv->mm.userfault_lock);
+   /*
+* Only called during RPM suspend. All users of the userfault_list
+* must be holding an RPM wakeref to ensure that this can not
+* run concurrently with themselves (and use the struct_mutex for
+* protection between themselves).
+*/
 
+   list_for_each_entry_safe(obj, on,
+_priv->mm.userfault_list, userfault_link) {
+   list_del_init(>userfault_link);
drm_vma_node_unmap(>base.vma_node,
   obj->base.dev->anon_inode->i_mapping);
-
-   spin_lock(_priv->mm.userfault_lock);
}
-   spin_unlock(_priv->mm.userfault_lock);
 }
 
 /**
@@ -4534,7 +4526,6 @@ int i915_gem_init(struct drm_device *dev)
int ret;
 
mutex_lock(>struct_mutex);
-   spin_lock_init(_priv->mm.userfault_lock);
 
if (!i915.enable_execlists) {
dev_priv->gt.resume = intel_legacy_submission_resume;
-- 
2.9.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx