| Message ID | 20180328114144.19890-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/03/18 14:41, Daniel Vetter wrote: > The only thing that omap_gem_free_object does that might need the > magic protection of struct_mutex (of keeping all objects alive if that > lock is held, even if the last reference is gone) is the mm_list > manipulation. > > But that is already protected by the separate omapdrm->list_lock, > which means struct_mutex is an entirely internal lock for omapdrm. > Everything else is just releasing resources, which is all protected > already by the various subsystems and allocators. > > To make this even more obvious we could do an > s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But > since omapdrm is a lot bigger and a lot more active I'll refrain from > that - this is better done by omapdrm developers at some suitable time > in the future. > > v2: Just auditing the code isn't enough, I actually have to remove > the now wrong locking check in omap_gem_free_object ... > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> This version works fine. I'll pick this to omapdrm branch. Thanks! Tomi
Hello, On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote: > On 28/03/18 14:41, Daniel Vetter wrote: > > The only thing that omap_gem_free_object does that might need the > > magic protection of struct_mutex (of keeping all objects alive if that > > lock is held, even if the last reference is gone) is the mm_list > > manipulation. > > > > But that is already protected by the separate omapdrm->list_lock, > > which means struct_mutex is an entirely internal lock for omapdrm. > > Everything else is just releasing resources, which is all protected > > already by the various subsystems and allocators. > > > > To make this even more obvious we could do an > > s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But > > since omapdrm is a lot bigger and a lot more active I'll refrain from > > that - this is better done by omapdrm developers at some suitable time > > in the future. > > > > v2: Just auditing the code isn't enough, I actually have to remove > > the now wrong locking check in omap_gem_free_object ... > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > > This version works fine. I'll pick this to omapdrm branch. Thanks! Unless I'm mistaken (this is only based on code analysis), a WARN_ON could also be triggered through the following call stack. gem_free_object_unlocked() omap_gem_free_object() evict() evict_entry() mmap_offset() WARN_ON(!mutex_is_locked(&dev->struct_mutex)) There could be other such call stacks. I don't think we should switch to gem_free_object_unlocked() until all usage of struct_mutex is removed from the omapdrm driver.
On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote: > Hello, > > On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote: > > On 28/03/18 14:41, Daniel Vetter wrote: > > > The only thing that omap_gem_free_object does that might need the > > > magic protection of struct_mutex (of keeping all objects alive if that > > > lock is held, even if the last reference is gone) is the mm_list > > > manipulation. > > > > > > But that is already protected by the separate omapdrm->list_lock, > > > which means struct_mutex is an entirely internal lock for omapdrm. > > > Everything else is just releasing resources, which is all protected > > > already by the various subsystems and allocators. > > > > > > To make this even more obvious we could do an > > > s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But > > > since omapdrm is a lot bigger and a lot more active I'll refrain from > > > that - this is better done by omapdrm developers at some suitable time > > > in the future. > > > > > > v2: Just auditing the code isn't enough, I actually have to remove > > > the now wrong locking check in omap_gem_free_object ... > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > This version works fine. I'll pick this to omapdrm branch. Thanks! > > Unless I'm mistaken (this is only based on code analysis), a WARN_ON could > also be triggered through the following call stack. > > gem_free_object_unlocked() > omap_gem_free_object() > evict() > evict_entry() > mmap_offset() > WARN_ON(!mutex_is_locked(&dev->struct_mutex)) > > There could be other such call stacks. > > I don't think we should switch to gem_free_object_unlocked() until all usage > of struct_mutex is removed from the omapdrm driver. Hm, indeed I missed that one. I'm not entirely sure how the tiler locking will pan out though, and whether there's not some stuff that won't be protected anymore. Specifically omapdrm_priv->usergart looks like it actually requires struct_mutex :-/ -Daniel > > -- > Regards, > > Laurent Pinchart > > >
Hi Daniel, On Tuesday, 3 April 2018 12:07:31 EEST Daniel Vetter wrote: > On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote: > > On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote: > >> On 28/03/18 14:41, Daniel Vetter wrote: > >>> The only thing that omap_gem_free_object does that might need the > >>> magic protection of struct_mutex (of keeping all objects alive if that > >>> lock is held, even if the last reference is gone) is the mm_list > >>> manipulation. > >>> > >>> But that is already protected by the separate omapdrm->list_lock, > >>> which means struct_mutex is an entirely internal lock for omapdrm. > >>> Everything else is just releasing resources, which is all protected > >>> already by the various subsystems and allocators. > >>> > >>> To make this even more obvious we could do an > >>> s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But > >>> since omapdrm is a lot bigger and a lot more active I'll refrain from > >>> that - this is better done by omapdrm developers at some suitable time > >>> in the future. > >>> > >>> v2: Just auditing the code isn't enough, I actually have to remove > >>> the now wrong locking check in omap_gem_free_object ... > >>> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > >> > >> This version works fine. I'll pick this to omapdrm branch. Thanks! > > > > Unless I'm mistaken (this is only based on code analysis), a WARN_ON could > > also be triggered through the following call stack. > > > > gem_free_object_unlocked() > > omap_gem_free_object() > > evict() > > evict_entry() > > mmap_offset() > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)) > > > > There could be other such call stacks. > > > > I don't think we should switch to gem_free_object_unlocked() until all > > usage of struct_mutex is removed from the omapdrm driver. > > Hm, indeed I missed that one. I'm not entirely sure how the tiler locking > will pan out though, and whether there's not some stuff that won't be > protected anymore. I reviewed locking through the driver when writing "drm/omap: gem: Replace struct_mutex usage with omap_obj private lock", but I'm also concerned that I might have missed something and introduced a race condition :-/ > Specifically omapdrm_priv->usergart looks like it actually requires > struct_mutex :-/ I think you're right. Or at rather not struct_mutex, but an omapdrm-specific gart mutex. I'm not too familiar with that code, I suppose I'll have to dive in.
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 5fcf9eaf3eaf..5005ecc284d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -493,7 +493,7 @@ static struct drm_driver omap_drm_driver = { .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = omap_gem_prime_export, .gem_prime_import = omap_gem_prime_import, - .gem_free_object = omap_gem_free_object, + .gem_free_object_unlocked = omap_gem_free_object, .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset, diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 183447572e90..ac4175c13ecb 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1086,8 +1086,6 @@ void omap_gem_free_object(struct drm_gem_object *obj) evict(obj); - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - mutex_lock(&priv->list_lock); list_del(&omap_obj->mm_list); mutex_unlock(&priv->list_lock);
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation. But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators. To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future. v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ... Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)