[PATCH] drm: Don't reference objects in the flink name idr
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 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 > > > > 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 Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm: Don't reference objects in the flink name idr
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
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(>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(>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(>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 at 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
[PATCH] drm: Don't reference objects in the flink name idr
On Tue, Dec 3, 2013 at 7:26 AM, Daniel Vetter 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
>
> 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(>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(>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(>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 at 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
[PATCH] drm: Don't reference objects in the flink name idr
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
---
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(>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(>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(>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
