[PATCH] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Chris Wilson]

On Thu, Oct 15, 2015 at 11:33:43AM +0200, Daniel Vetter wrote:
> Compared to wrapping the final kref_put with dev->struct_mutex this
> allows us to only acquire the offset manager look both in the final
> cleanup and in the lookup. Which has the upside that no locks leak out
> of the core abstractions. But it means that we need to hold a
> temporary reference to the object while checking mmap constraints, to
> make sure the object doesn't disappear. Extended the critical region
> would have worked too, but would result in more leaky locking.
> 
> Also, this is the final bit which required dev->struct_mutex in gem
> core, now modern drivers can be completely struct_mutex free!
> 
> This needs a new drm_vma_offset_exact_lookup_locked and makes both
> drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
> 
> v2: Don't leak object references in failure paths (David).
> 
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Reviewed-by: David Herrmann <dh.herrmann at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/drm_gem.c         | 30 +++++++++++++++++------------
>  drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
>  include/drm/drm_vma_manager.h     | 22 ++++++---------------
>  3 files changed, 36 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ab8ea42264f4..663fadbe979e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
>  	struct drm_file *priv = filp->private_data;
>  	struct drm_device *dev = priv->minor->dev;
> -	struct drm_gem_object *obj;
> +	struct drm_gem_object *obj = NULL;
>  	struct drm_vma_offset_node *node;
>  	int ret;
>  
>  	if (drm_device_is_unplugged(dev))
>  		return -ENODEV;
>  

/* bla bla bla
 * When the object is being freed, after it hits 0-refcnt
 * it acquires the struct_mutex and proceeds to tear down
 * the object. In the process it will attempt to remove
 * the VMA offset and so acquire this mgr->vm_lock.
 * Therefore if we find an object with a 0-refcnt that matches
 * our range, we know it is in the process of being destroyed
 * and will be freed as soon as we release the lock - so
 * we have to check for the 0-refcnted object and treat it as
 * invalid.
 */
> -	mutex_lock(&dev->struct_mutex);
> +	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> +	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> +						  vma->vm_pgoff,
> +						  vma_pages(vma));
> +	if (likely(node)) {
> +		obj = container_of(node, struct drm_gem_object, vma_node);
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			obj = NULL;
> +	}
> +	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre