| Message ID | 1473300477-2864-1-git-send-email-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 07, 2016 at 10:07:57PM -0400, Huang Rui wrote: > In previous drm_global_item_ref, there are two times of writing > ref->object if item->refcount is 0. So this patch does a minor update > to put alloc and init ref firstly, and then to modify the item of glob > array. Use "else" to avoid two times of writing ref->object. It can > make the code logic more clearly. > > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > Changes from V2 -> V3: > - Use duplicate mutex release to avoid "goto" in non-error patch. > - Rename error label > > --- > drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c > index 3d2e91c..b404287 100644 > --- a/drivers/gpu/drm/drm_global.c > +++ b/drivers/gpu/drm/drm_global.c > @@ -65,30 +65,34 @@ void drm_global_release(void) > > int drm_global_item_ref(struct drm_global_reference *ref) > { > - int ret; > + int ret = 0; > struct drm_global_item *item = &glob[ref->global_type]; > > mutex_lock(&item->mutex); > if (item->refcount == 0) { > - item->object = kzalloc(ref->size, GFP_KERNEL); > - if (unlikely(item->object == NULL)) { > + ref->object = kzalloc(ref->size, GFP_KERNEL); So the item refcount is 0, we operate on ref, whereas previous we inspected item and operated on item. Not an improvement. -Chris
On Thu, Sep 08, 2016 at 02:36:06PM +0800, Chris Wilson wrote: > On Wed, Sep 07, 2016 at 10:07:57PM -0400, Huang Rui wrote: > > In previous drm_global_item_ref, there are two times of writing > > ref->object if item->refcount is 0. So this patch does a minor update > > to put alloc and init ref firstly, and then to modify the item of glob > > array. Use "else" to avoid two times of writing ref->object. It can > > make the code logic more clearly. > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > Changes from V2 -> V3: > > - Use duplicate mutex release to avoid "goto" in non-error patch. > > - Rename error label > > > > --- > > drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- > > 1 file changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c > > index 3d2e91c..b404287 100644 > > --- a/drivers/gpu/drm/drm_global.c > > +++ b/drivers/gpu/drm/drm_global.c > > @@ -65,30 +65,34 @@ void drm_global_release(void) > > > > int drm_global_item_ref(struct drm_global_reference *ref) > > { > > - int ret; > > + int ret = 0; > > struct drm_global_item *item = &glob[ref->global_type]; > > > > mutex_lock(&item->mutex); > > if (item->refcount == 0) { > > - item->object = kzalloc(ref->size, GFP_KERNEL); > > - if (unlikely(item->object == NULL)) { > > + ref->object = kzalloc(ref->size, GFP_KERNEL); > > So the item refcount is 0, we operate on ref, whereas previous we > inspected item and operated on item. Not an improvement. Hmm, when item refcount is 0, we operate on ref to create object and init it for item, so although item->object and ref->object here should point the same thing, but we should alloc and init ref firstly and pass the ref->object address to item (item actually points a static area). This make the code logic more clearly and readable. So I updated it to "ref" here. :-) Thanks, Rui
Am 08.09.2016 um 04:07 schrieb Huang Rui: > In previous drm_global_item_ref, there are two times of writing > ref->object if item->refcount is 0. So this patch does a minor update > to put alloc and init ref firstly, and then to modify the item of glob > array. Use "else" to avoid two times of writing ref->object. It can > make the code logic more clearly. > > Signed-off-by: Huang Rui <ray.huang@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com>. > --- > > Changes from V2 -> V3: > - Use duplicate mutex release to avoid "goto" in non-error patch. > - Rename error label > > --- > drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c > index 3d2e91c..b404287 100644 > --- a/drivers/gpu/drm/drm_global.c > +++ b/drivers/gpu/drm/drm_global.c > @@ -65,30 +65,34 @@ void drm_global_release(void) > > int drm_global_item_ref(struct drm_global_reference *ref) > { > - int ret; > + int ret = 0; > struct drm_global_item *item = &glob[ref->global_type]; > > mutex_lock(&item->mutex); > if (item->refcount == 0) { > - item->object = kzalloc(ref->size, GFP_KERNEL); > - if (unlikely(item->object == NULL)) { > + ref->object = kzalloc(ref->size, GFP_KERNEL); > + if (unlikely(ref->object == NULL)) { > ret = -ENOMEM; > - goto out_err; > + goto error_unlock; > } > - > - ref->object = item->object; > ret = ref->init(ref); > if (unlikely(ret != 0)) > - goto out_err; > + goto error_free; > > + item->object = ref->object; > + } else { > + ref->object = item->object; > } > + > ++item->refcount; > - ref->object = item->object; > mutex_unlock(&item->mutex); > return 0; > -out_err: > + > +error_free: > + kfree(ref->object); > + ref->object = NULL; > +error_unlock: > mutex_unlock(&item->mutex); > - item->object = NULL; > return ret; > } > EXPORT_SYMBOL(drm_global_item_ref);
On Thu, Sep 08, 2016 at 03:22:48PM +0800, Huang Rui wrote: > On Thu, Sep 08, 2016 at 02:36:06PM +0800, Chris Wilson wrote: > > On Wed, Sep 07, 2016 at 10:07:57PM -0400, Huang Rui wrote: > > > In previous drm_global_item_ref, there are two times of writing > > > ref->object if item->refcount is 0. So this patch does a minor update > > > to put alloc and init ref firstly, and then to modify the item of glob > > > array. Use "else" to avoid two times of writing ref->object. It can > > > make the code logic more clearly. > > > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > --- > > > > > > Changes from V2 -> V3: > > > - Use duplicate mutex release to avoid "goto" in non-error patch. > > > - Rename error label > > > > > > --- > > > drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- > > > 1 file changed, 14 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c > > > index 3d2e91c..b404287 100644 > > > --- a/drivers/gpu/drm/drm_global.c > > > +++ b/drivers/gpu/drm/drm_global.c > > > @@ -65,30 +65,34 @@ void drm_global_release(void) > > > > > > int drm_global_item_ref(struct drm_global_reference *ref) > > > { > > > - int ret; > > > + int ret = 0; > > > struct drm_global_item *item = &glob[ref->global_type]; > > > > > > mutex_lock(&item->mutex); > > > if (item->refcount == 0) { > > > - item->object = kzalloc(ref->size, GFP_KERNEL); > > > - if (unlikely(item->object == NULL)) { > > > + ref->object = kzalloc(ref->size, GFP_KERNEL); > > > > So the item refcount is 0, we operate on ref, whereas previous we > > inspected item and operated on item. Not an improvement. > > Hmm, when item refcount is 0, we operate on ref to create object and init > it for item, so although item->object and ref->object here should point the > same thing, but we should alloc and init ref firstly and pass the > ref->object address to item (item actually points a static area). This make > the code logic more clearly and readable. So I updated it to "ref" here. > :-) The object is owned by item. ref is just a pointer to it. -Chris
Am 08.09.2016 um 09:35 schrieb Chris Wilson: > On Thu, Sep 08, 2016 at 03:22:48PM +0800, Huang Rui wrote: >> On Thu, Sep 08, 2016 at 02:36:06PM +0800, Chris Wilson wrote: >>> On Wed, Sep 07, 2016 at 10:07:57PM -0400, Huang Rui wrote: >>>> In previous drm_global_item_ref, there are two times of writing >>>> ref->object if item->refcount is 0. So this patch does a minor update >>>> to put alloc and init ref firstly, and then to modify the item of glob >>>> array. Use "else" to avoid two times of writing ref->object. It can >>>> make the code logic more clearly. >>>> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> >>>> Changes from V2 -> V3: >>>> - Use duplicate mutex release to avoid "goto" in non-error patch. >>>> - Rename error label >>>> >>>> --- >>>> drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- >>>> 1 file changed, 14 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c >>>> index 3d2e91c..b404287 100644 >>>> --- a/drivers/gpu/drm/drm_global.c >>>> +++ b/drivers/gpu/drm/drm_global.c >>>> @@ -65,30 +65,34 @@ void drm_global_release(void) >>>> >>>> int drm_global_item_ref(struct drm_global_reference *ref) >>>> { >>>> - int ret; >>>> + int ret = 0; >>>> struct drm_global_item *item = &glob[ref->global_type]; >>>> >>>> mutex_lock(&item->mutex); >>>> if (item->refcount == 0) { >>>> - item->object = kzalloc(ref->size, GFP_KERNEL); >>>> - if (unlikely(item->object == NULL)) { >>>> + ref->object = kzalloc(ref->size, GFP_KERNEL); >>> So the item refcount is 0, we operate on ref, whereas previous we >>> inspected item and operated on item. Not an improvement. >> Hmm, when item refcount is 0, we operate on ref to create object and init >> it for item, so although item->object and ref->object here should point the >> same thing, but we should alloc and init ref firstly and pass the >> ref->object address to item (item actually points a static area). This make >> the code logic more clearly and readable. So I updated it to "ref" here. >> :-) > The object is owned by item. ref is just a pointer to it. Yeah, so what? We initialized ref->item when the refcount was 0 before as well. Why not create the reference first and initialize the item later on? Regards, Christian. > -Chris >
On Thu, Sep 08, 2016 at 09:43:52AM +0200, Christian König wrote: > Am 08.09.2016 um 09:35 schrieb Chris Wilson: > >On Thu, Sep 08, 2016 at 03:22:48PM +0800, Huang Rui wrote: > >>On Thu, Sep 08, 2016 at 02:36:06PM +0800, Chris Wilson wrote: > >>>On Wed, Sep 07, 2016 at 10:07:57PM -0400, Huang Rui wrote: > >>>>In previous drm_global_item_ref, there are two times of writing > >>>>ref->object if item->refcount is 0. So this patch does a minor update > >>>>to put alloc and init ref firstly, and then to modify the item of glob > >>>>array. Use "else" to avoid two times of writing ref->object. It can > >>>>make the code logic more clearly. > >>>> > >>>>Signed-off-by: Huang Rui <ray.huang@amd.com> > >>>>--- > >>>> > >>>>Changes from V2 -> V3: > >>>>- Use duplicate mutex release to avoid "goto" in non-error patch. > >>>>- Rename error label > >>>> > >>>>--- > >>>> drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- > >>>> 1 file changed, 14 insertions(+), 10 deletions(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c > >>>>index 3d2e91c..b404287 100644 > >>>>--- a/drivers/gpu/drm/drm_global.c > >>>>+++ b/drivers/gpu/drm/drm_global.c > >>>>@@ -65,30 +65,34 @@ void drm_global_release(void) > >>>> int drm_global_item_ref(struct drm_global_reference *ref) > >>>> { > >>>>- int ret; > >>>>+ int ret = 0; > >>>> struct drm_global_item *item = &glob[ref->global_type]; > >>>> mutex_lock(&item->mutex); > >>>> if (item->refcount == 0) { > >>>>- item->object = kzalloc(ref->size, GFP_KERNEL); > >>>>- if (unlikely(item->object == NULL)) { > >>>>+ ref->object = kzalloc(ref->size, GFP_KERNEL); > >>>So the item refcount is 0, we operate on ref, whereas previous we > >>>inspected item and operated on item. Not an improvement. > >>Hmm, when item refcount is 0, we operate on ref to create object and init > >>it for item, so although item->object and ref->object here should point the > >>same thing, but we should alloc and init ref firstly and pass the > >>ref->object address to item (item actually points a static area). This make > >>the code logic more clearly and readable. So I updated it to "ref" here. > >>:-) > >The object is owned by item. ref is just a pointer to it. > > Yeah, so what? We initialized ref->item when the refcount was 0 > before as well. > > Why not create the reference first and initialize the item later on? More to the point, there is a genuine bug in the code. It happens to be fixed by this patch, but not purported to be. There is also a leak of item->object, and the globals conflict between drivers. If radeon instantiates the DRM_GLOBAL_TTM_MEM, nouveau bumps the reference and radeon subsequently unloaded, when nouveau unloads it will crash due to executing a random address. As a singleton class for ttm, it could do with a revamp. -Chris
diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c index 3d2e91c..b404287 100644 --- a/drivers/gpu/drm/drm_global.c +++ b/drivers/gpu/drm/drm_global.c @@ -65,30 +65,34 @@ void drm_global_release(void) int drm_global_item_ref(struct drm_global_reference *ref) { - int ret; + int ret = 0; struct drm_global_item *item = &glob[ref->global_type]; mutex_lock(&item->mutex); if (item->refcount == 0) { - item->object = kzalloc(ref->size, GFP_KERNEL); - if (unlikely(item->object == NULL)) { + ref->object = kzalloc(ref->size, GFP_KERNEL); + if (unlikely(ref->object == NULL)) { ret = -ENOMEM; - goto out_err; + goto error_unlock; } - - ref->object = item->object; ret = ref->init(ref); if (unlikely(ret != 0)) - goto out_err; + goto error_free; + item->object = ref->object; + } else { + ref->object = item->object; } + ++item->refcount; - ref->object = item->object; mutex_unlock(&item->mutex); return 0; -out_err: + +error_free: + kfree(ref->object); + ref->object = NULL; +error_unlock: mutex_unlock(&item->mutex); - item->object = NULL; return ret; } EXPORT_SYMBOL(drm_global_item_ref);
In previous drm_global_item_ref, there are two times of writing ref->object if item->refcount is 0. So this patch does a minor update to put alloc and init ref firstly, and then to modify the item of glob array. Use "else" to avoid two times of writing ref->object. It can make the code logic more clearly. Signed-off-by: Huang Rui <ray.huang@amd.com> --- Changes from V2 -> V3: - Use duplicate mutex release to avoid "goto" in non-error patch. - Rename error label --- drivers/gpu/drm/drm_global.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)