diff mbox

drm/gem: Use kref_get_unless_zero for the weak mmap references

Message ID 1444901623-18918-1-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter Oct. 15, 2015, 9:33 a.m. UTC
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@gmail.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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(-)

Comments

Chris Wilson Oct. 15, 2015, 10:06 a.m. UTC | #1
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@gmail.com>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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@chris-wilson.co.uk>
-Chris
Daniel Vetter Oct. 15, 2015, 11:11 a.m. UTC | #2
On Thu, Oct 15, 2015 at 11:06:59AM +0100, Chris Wilson wrote:
> 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@gmail.com>
> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.
>  */

As discussed on irc struct_mutex is just a digression, so I left that part
out when adding your comment.

> > -	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@chris-wilson.co.uk>

All four core gem patches now merged in drm-misc, thanks for the review.
-Daniel
Daniel Vetter Oct. 15, 2015, 12:31 p.m. UTC | #3
On Thu, Oct 15, 2015 at 01:11:36PM +0200, Daniel Vetter wrote:
> On Thu, Oct 15, 2015 at 11:06:59AM +0100, Chris Wilson wrote:
> > 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@gmail.com>
> > > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> >  */
> 
> As discussed on irc struct_mutex is just a digression, so I left that part
> out when adding your comment.
> 
> > > -	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@chris-wilson.co.uk>
> 
> All four core gem patches now merged in drm-misc, thanks for the review.

Argh, last patch needs one of the vgem ones as prep, so dropped it again.
-Daniel
diff mbox

Patch

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;
 
-	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);
 
-	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
-					   vma->vm_pgoff,
-					   vma_pages(vma));
-	if (!node) {
-		mutex_unlock(&dev->struct_mutex);
+	if (!obj)
 		return -EINVAL;
-	} else if (!drm_vma_node_is_allowed(node, filp)) {
-		mutex_unlock(&dev->struct_mutex);
+
+	if (!drm_vma_node_is_allowed(node, filp)) {
+		drm_gem_object_unreference_unlocked(obj);
 		return -EACCES;
 	}
 
-	obj = container_of(node, struct drm_gem_object, vma_node);
-	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
+	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
+			       vma);
 
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
index 68c1f32fb086..2f2ecde8285b 100644
--- a/drivers/gpu/drm/drm_vma_manager.c
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -112,7 +112,7 @@  void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
 EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
 
 /**
- * drm_vma_offset_lookup() - Find node in offset space
+ * drm_vma_offset_lookup_locked() - Find node in offset space
  * @mgr: Manager object
  * @start: Start address for object (page-based)
  * @pages: Size of object (page-based)
@@ -122,37 +122,21 @@  EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
  * region and the given node will be returned, as long as the node spans the
  * whole requested area (given the size in number of pages as @pages).
  *
- * RETURNS:
- * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned. It's the caller's responsibility to make sure the node doesn't
- * get destroyed before the caller can access it.
- */
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages)
-{
-	struct drm_vma_offset_node *node;
-
-	read_lock(&mgr->vm_lock);
-	node = drm_vma_offset_lookup_locked(mgr, start, pages);
-	read_unlock(&mgr->vm_lock);
-
-	return node;
-}
-EXPORT_SYMBOL(drm_vma_offset_lookup);
-
-/**
- * drm_vma_offset_lookup_locked() - Find node in offset space
- * @mgr: Manager object
- * @start: Start address for object (page-based)
- * @pages: Size of object (page-based)
+ * Note that before lookup the vma offset manager lookup lock must be acquired
+ * with drm_vma_offset_lock_lookup(). See there for an example. This can then be
+ * used to implement weakly referenced lookups using kref_get_unless_zero().
  *
- * Same as drm_vma_offset_lookup() but requires the caller to lock offset lookup
- * manually. See drm_vma_offset_lock_lookup() for an example.
+ * Example:
+ *     drm_vma_offset_lock_lookup(mgr);
+ *     node = drm_vma_offset_lookup_locked(mgr);
+ *     if (node)
+ *         kref_get_unless_zero(container_of(node, sth, entr));
+ *     drm_vma_offset_unlock_lookup(mgr);
  *
  * RETURNS:
  * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned.
+ * is returned. It's the caller's responsibility to make sure the node doesn't
+ * get destroyed before the caller can access it.
  */
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							 unsigned long start,
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 74f3b38c43c1..2f63dd5e05eb 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -54,9 +54,6 @@  void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
 				 unsigned long page_offset, unsigned long size);
 void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
 
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages);
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							   unsigned long start,
 							   unsigned long pages);
@@ -71,25 +68,25 @@  bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
 			     struct file *filp);
 
 /**
- * drm_vma_offset_exact_lookup() - Look up node by exact address
+ * drm_vma_offset_exact_lookup_locked() - Look up node by exact address
  * @mgr: Manager object
  * @start: Start address (page-based, not byte-based)
  * @pages: Size of object (page-based)
  *
- * Same as drm_vma_offset_lookup() but does not allow any offset into the node.
+ * Same as drm_vma_offset_lookup_locked() but does not allow any offset into the node.
  * It only returns the exact object with the given start address.
  *
  * RETURNS:
  * Node at exact start address @start.
  */
 static inline struct drm_vma_offset_node *
-drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
-			    unsigned long start,
-			    unsigned long pages)
+drm_vma_offset_exact_lookup_locked(struct drm_vma_offset_manager *mgr,
+				   unsigned long start,
+				   unsigned long pages)
 {
 	struct drm_vma_offset_node *node;
 
-	node = drm_vma_offset_lookup(mgr, start, pages);
+	node = drm_vma_offset_lookup_locked(mgr, start, pages);
 	return (node && node->vm_node.start == start) ? node : NULL;
 }
 
@@ -108,13 +105,6 @@  drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
  * not call any other VMA helpers while holding this lock.
  *
  * Note: You're in atomic-context while holding this lock!
- *
- * Example:
- *     drm_vma_offset_lock_lookup(mgr);
- *     node = drm_vma_offset_lookup_locked(mgr);
- *     if (node)
- *         kref_get_unless_zero(container_of(node, sth, entr));
- *     drm_vma_offset_unlock_lookup(mgr);
  */
 static inline void drm_vma_offset_lock_lookup(struct drm_vma_offset_manager *mgr)
 {