| Message ID | 1373958731-4132-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Only one callsite and since ->handle_count is not a simple reference > count (it can resurrect) it's imo better to be explicit about things > than hide the refcount dance. I'm not really sure I like this one.. I guess it could be that I'm just used to the handle-ref stuff, so it doesn't seem odd not-inlined. And it does seem kinda odd / unsymmetric to have an unref w/out a ref. I guess I kinda like the bikeshed's current color in this case. BR, -R > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_gem.c | 3 ++- > include/drm/drmP.h | 7 ------- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 603f256..7bcd851 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -280,7 +280,8 @@ drm_gem_handle_create(struct drm_file *file_priv, > return ret; > *handlep = ret; > > - drm_gem_object_handle_reference(obj); > + drm_gem_object_reference(obj); > + atomic_inc(&obj->handle_count); > > if (dev->driver->gem_open_object) { > ret = dev->driver->gem_open_object(obj, file_priv); > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index f949cb2..114db57 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1664,13 +1664,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, > int drm_gem_handle_delete(struct drm_file *filp, u32 handle); > > static inline void > -drm_gem_object_handle_reference(struct drm_gem_object *obj) > -{ > - drm_gem_object_reference(obj); > - atomic_inc(&obj->handle_count); > -} > - > -static inline void > drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) > { > if (obj == NULL) > -- > 1.8.3.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jul 23, 2013 at 2:07 PM, Rob Clark <robdclark@gmail.com> wrote: > On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> Only one callsite and since ->handle_count is not a simple reference >> count (it can resurrect) it's imo better to be explicit about things >> than hide the refcount dance. > > I'm not really sure I like this one.. I guess it could be that I'm > just used to the handle-ref stuff, so it doesn't seem odd not-inlined. > And it does seem kinda odd / unsymmetric to have an unref w/out a > ref. I guess I kinda like the bikeshed's current color in this case. I generally agree but in this case a follow-up patch ("drm/gem: fix up flink name create race") will change obj->handle_count from an atomic_t to a normal int protected by the dev->object_name_lock spinlock. To avoid races we need to hold that spinlock over a few different instructions, so with the refcounting dance inlined it's much more obvious that that obj->handle_count is always correctly protected imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jul 23, 2013 at 8:31 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Tue, Jul 23, 2013 at 2:07 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>> Only one callsite and since ->handle_count is not a simple reference >>> count (it can resurrect) it's imo better to be explicit about things >>> than hide the refcount dance. >> >> I'm not really sure I like this one.. I guess it could be that I'm >> just used to the handle-ref stuff, so it doesn't seem odd not-inlined. >> And it does seem kinda odd / unsymmetric to have an unref w/out a >> ref. I guess I kinda like the bikeshed's current color in this case. > > I generally agree but in this case a follow-up patch ("drm/gem: fix up > flink name create race") will change obj->handle_count from an > atomic_t to a normal int protected by the dev->object_name_lock > spinlock. To avoid races we need to hold that spinlock over a few > different instructions, so with the refcounting dance inlined it's > much more obvious that that obj->handle_count is always correctly > protected imo. hmm, ok.. then I'll reserve judgment until I get further through the series ;-) if you do spin another version of the series, it could be worth mentioning this in the commit msg BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jul 23, 2013 at 10:43 PM, Rob Clark <robdclark@gmail.com> wrote: > On Tue, Jul 23, 2013 at 8:31 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> On Tue, Jul 23, 2013 at 2:07 PM, Rob Clark <robdclark@gmail.com> wrote: >>> On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>>> Only one callsite and since ->handle_count is not a simple reference >>>> count (it can resurrect) it's imo better to be explicit about things >>>> than hide the refcount dance. >>> >>> I'm not really sure I like this one.. I guess it could be that I'm >>> just used to the handle-ref stuff, so it doesn't seem odd not-inlined. >>> And it does seem kinda odd / unsymmetric to have an unref w/out a >>> ref. I guess I kinda like the bikeshed's current color in this case. >> >> I generally agree but in this case a follow-up patch ("drm/gem: fix up >> flink name create race") will change obj->handle_count from an >> atomic_t to a normal int protected by the dev->object_name_lock >> spinlock. To avoid races we need to hold that spinlock over a few >> different instructions, so with the refcounting dance inlined it's >> much more obvious that that obj->handle_count is always correctly >> protected imo. > > hmm, ok.. then I'll reserve judgment until I get further through the series ;-) > > if you do spin another version of the series, it could be worth > mentioning this in the commit msg > Just merge this into that series or patch, deinlining it now it just noise. Dave.
On Wed, Jul 24, 2013 at 2:00 AM, Dave Airlie <airlied@gmail.com> wrote: > On Tue, Jul 23, 2013 at 10:43 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Tue, Jul 23, 2013 at 8:31 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>> On Tue, Jul 23, 2013 at 2:07 PM, Rob Clark <robdclark@gmail.com> wrote: >>>> On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>>>> Only one callsite and since ->handle_count is not a simple reference >>>>> count (it can resurrect) it's imo better to be explicit about things >>>>> than hide the refcount dance. >>>> >>>> I'm not really sure I like this one.. I guess it could be that I'm >>>> just used to the handle-ref stuff, so it doesn't seem odd not-inlined. >>>> And it does seem kinda odd / unsymmetric to have an unref w/out a >>>> ref. I guess I kinda like the bikeshed's current color in this case. >>> >>> I generally agree but in this case a follow-up patch ("drm/gem: fix up >>> flink name create race") will change obj->handle_count from an >>> atomic_t to a normal int protected by the dev->object_name_lock >>> spinlock. To avoid races we need to hold that spinlock over a few >>> different instructions, so with the refcounting dance inlined it's >>> much more obvious that that obj->handle_count is always correctly >>> protected imo. >> >> hmm, ok.. then I'll reserve judgment until I get further through the series ;-) >> >> if you do spin another version of the series, it could be worth >> mentioning this in the commit msg >> > > Just merge this into that series or patch, > > deinlining it now it just noise. Yeah, I'll do so. I've started with a few refactoring patches to get a clearer picture (since I've implemented&dismissed a few approaches to fix the flink race until I've settled on this one here), but now this patch here really looks a bit silly ;-) -Danie -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 603f256..7bcd851 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -280,7 +280,8 @@ drm_gem_handle_create(struct drm_file *file_priv, return ret; *handlep = ret; - drm_gem_object_handle_reference(obj); + drm_gem_object_reference(obj); + atomic_inc(&obj->handle_count); if (dev->driver->gem_open_object) { ret = dev->driver->gem_open_object(obj, file_priv); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f949cb2..114db57 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1664,13 +1664,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, int drm_gem_handle_delete(struct drm_file *filp, u32 handle); static inline void -drm_gem_object_handle_reference(struct drm_gem_object *obj) -{ - drm_gem_object_reference(obj); - atomic_inc(&obj->handle_count); -} - -static inline void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (obj == NULL)
Only one callsite and since ->handle_count is not a simple reference count (it can resurrect) it's imo better to be explicit about things than hide the refcount dance. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_gem.c | 3 ++- include/drm/drmP.h | 7 ------- 2 files changed, 2 insertions(+), 8 deletions(-)