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