Re: [PATCH 3/3] drm/i915: Remove superfluous locking around userfault_list

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 11, 2016 at 03:37:59PM +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 <chris@chris-wilson.co.uk>

So this is a bit a pet peeve of mine, but when changing locking schemes
I'm trying to make an as clear as possible distinction between locking
(which is just meant to protect the integrity of data structures, like
e.g. the userfault_list here). And anything else which manages lifetimes
and resources (all the get/put stuff we have) or ordering/sequence
(waiting, waking up other stuff and all that).

The reason for that is that with a BKL it is super easy to smash all these
things into one, but if you have locking that also keeps stuff alive and
guarantees ordering (or the other way round) it becomes nigh to impossible
to untangle.

Hence why I don't like this.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 ---
>  drivers/gpu/drm/i915/i915_gem.c | 21 ++++-----------------
>  2 files changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72b3126c6c74..13ca968a760a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1359,9 +1359,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 587a91af5a3f..a268e804005c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1853,9 +1853,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>  		goto err_unpin;
>  
>  	assert_rpm_wakelock_held(dev_priv);
> -	spin_lock(&dev_priv->mm.userfault_lock);
>  	list_add(&obj->userfault_link, &dev_priv->mm.userfault_list);
> -	spin_unlock(&dev_priv->mm.userfault_lock);
>  
>  err_unpin:
>  	__i915_vma_unpin(vma);
> @@ -1925,7 +1923,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)
>  	 */
>  	assert_rpm_wakelock_held(i915);
>  
> -	spin_lock(&i915->mm.userfault_lock);
> -	if (!list_empty(&obj->userfault_link)) {
> -		list_del_init(&obj->userfault_link);
> -		zap = true;
> -	}
> -	spin_unlock(&i915->mm.userfault_lock);
> -	if (!zap)
> +	if (list_empty(&obj->userfault_link))
>  		return;
>  
> +	list_del_init(&obj->userfault_link);
>  	drm_vma_node_unmap(&obj->base.vma_node,
>  			   obj->base.dev->anon_inode->i_mapping);
>  
> @@ -1963,13 +1955,9 @@ void
>  i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_i915_gem_object *obj, *on;
> -	struct list_head userfault_list;
> -
> -	spin_lock(&dev_priv->mm.userfault_lock);
> -	list_replace_init(&dev_priv->mm.userfault_list, &userfault_list);
> -	spin_unlock(&dev_priv->mm.userfault_lock);
>  
> -	list_for_each_entry_safe(obj, on, &userfault_list, userfault_link)
> +	list_for_each_entry_safe(obj, on,
> +				 &dev_priv->mm.userfault_list, userfault_link)
>  		i915_gem_release_mmap(obj);
>  }
>  
> @@ -4457,7 +4445,6 @@ int i915_gem_init(struct drm_device *dev)
>  	int ret;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	spin_lock_init(&dev_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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx