| Message ID | 1386034577-9195-1-git-send-email-krh@bitplanet.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Dec 02, 2013 at 05:36:17PM -0800, Kristian Høgsberg wrote: > There's no reason to keep a reference to objects in the name idr. Each > handle to an object has a reference to the object and just before we > destroy the last handle we take the object out of the name idr. Thus, > if an object is in the name idr, there's at least one reference to the > object. > > Or to put it another way, the name idr reference will never keep the > object alive. It just looks like it, which is confusing. > > Signed-off-by: Kristian Høgsberg <krh@bitplanet.net> I expect this to blow up when you race gem_close ioctl calls with flink open. i-g-t/gem_flink_close tests actually have been written specifically to exercise these races. -Daniel > --- > drivers/gpu/drm/drm_gem.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 4761ade..3ff39bb 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -175,11 +175,6 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) > mutex_unlock(&filp->prime.lock); > } > > -static void drm_gem_object_ref_bug(struct kref *list_kref) > -{ > - BUG(); > -} > - > /** > * Called after the last handle to the object has been closed > * > @@ -195,13 +190,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) > if (obj->name) { > idr_remove(&dev->object_name_idr, obj->name); > obj->name = 0; > - /* > - * The object name held a reference to this object, drop > - * that now. > - * > - * This cannot be the last reference, since the handle holds one too. > - */ > - kref_put(&obj->refcount, drm_gem_object_ref_bug); > } > } > > @@ -602,9 +590,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, > goto err; > > obj->name = ret; > - > - /* Allocate a reference for the name table. */ > - drm_gem_object_reference(obj); > } > > args->name = (uint64_t) obj->name; > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 3, 2013 at 7:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Dec 02, 2013 at 05:36:17PM -0800, Kristian Høgsberg wrote: >> There's no reason to keep a reference to objects in the name idr. Each >> handle to an object has a reference to the object and just before we >> destroy the last handle we take the object out of the name idr. Thus, >> if an object is in the name idr, there's at least one reference to the >> object. >> >> Or to put it another way, the name idr reference will never keep the >> object alive. It just looks like it, which is confusing. >> >> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net> > > I expect this to blow up when you race gem_close ioctl calls with flink > open. i-g-t/gem_flink_close tests actually have been written specifically > to exercise these races. > -Daniel Can you be more specific about what race you see? The one thing that could go wrong is that the last handle is delete after we enter drm_gem_flink_ioctl() and look up the object but before taking the object_name_lock, but that's handled by checking obj->handle_count under the lock. Deleting handles and removing the name is always done under the object_name_lock from drm_gem_object_handle_unreference_unlocked(). Kristian >> --- >> drivers/gpu/drm/drm_gem.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 4761ade..3ff39bb 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -175,11 +175,6 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) >> mutex_unlock(&filp->prime.lock); >> } >> >> -static void drm_gem_object_ref_bug(struct kref *list_kref) >> -{ >> - BUG(); >> -} >> - >> /** >> * Called after the last handle to the object has been closed >> * >> @@ -195,13 +190,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) >> if (obj->name) { >> idr_remove(&dev->object_name_idr, obj->name); >> obj->name = 0; >> - /* >> - * The object name held a reference to this object, drop >> - * that now. >> - * >> - * This cannot be the last reference, since the handle holds one too. >> - */ >> - kref_put(&obj->refcount, drm_gem_object_ref_bug); >> } >> } >> >> @@ -602,9 +590,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, >> goto err; >> >> obj->name = ret; >> - >> - /* Allocate a reference for the name table. */ >> - drm_gem_object_reference(obj); >> } >> >> args->name = (uint64_t) obj->name; >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Dec 03, 2013 at 08:33:46AM -0800, Kristian Høgsberg wrote: > On Tue, Dec 3, 2013 at 7:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Dec 02, 2013 at 05:36:17PM -0800, Kristian Høgsberg wrote: > >> There's no reason to keep a reference to objects in the name idr. Each > >> handle to an object has a reference to the object and just before we > >> destroy the last handle we take the object out of the name idr. Thus, > >> if an object is in the name idr, there's at least one reference to the > >> object. > >> > >> Or to put it another way, the name idr reference will never keep the > >> object alive. It just looks like it, which is confusing. > >> > >> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net> > > > > I expect this to blow up when you race gem_close ioctl calls with flink > > open. i-g-t/gem_flink_close tests actually have been written specifically > > to exercise these races. > > -Daniel > > Can you be more specific about what race you see? The one thing that > could go wrong is that the last handle is delete after we enter > drm_gem_flink_ioctl() and look up the object but before taking the > object_name_lock, but that's handled by checking obj->handle_count > under the lock. Deleting handles and removing the name is always done > under the object_name_lock from > drm_gem_object_handle_unreference_unlocked(). Too many nightmares around lifetime rules, I've imagined some monster that isn't ther and stand corrected. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4761ade..3ff39bb 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -175,11 +175,6 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) mutex_unlock(&filp->prime.lock); } -static void drm_gem_object_ref_bug(struct kref *list_kref) -{ - BUG(); -} - /** * Called after the last handle to the object has been closed * @@ -195,13 +190,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) if (obj->name) { idr_remove(&dev->object_name_idr, obj->name); obj->name = 0; - /* - * The object name held a reference to this object, drop - * that now. - * - * This cannot be the last reference, since the handle holds one too. - */ - kref_put(&obj->refcount, drm_gem_object_ref_bug); } } @@ -602,9 +590,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, goto err; obj->name = ret; - - /* Allocate a reference for the name table. */ - drm_gem_object_reference(obj); } args->name = (uint64_t) obj->name;
There's no reason to keep a reference to objects in the name idr. Each handle to an object has a reference to the object and just before we destroy the last handle we take the object out of the name idr. Thus, if an object is in the name idr, there's at least one reference to the object. Or to put it another way, the name idr reference will never keep the object alive. It just looks like it, which is confusing. Signed-off-by: Kristian Høgsberg <krh@bitplanet.net> --- drivers/gpu/drm/drm_gem.c | 15 --------------- 1 file changed, 15 deletions(-)