[Updated,v2] drm: modify drm_global_item_ref to avoid two times of writing ref->object

Submitted by Huang, Ray on Sept. 7, 2016, 5:24 a.m.

Details

Message ID 1473225871-2847-1-git-send-email-ray.huang@amd.com
State New
Headers show
Series "drm: modify drm_global_item_ref to avoid two times of writing ref->object" ( rev: 4 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Huang, Ray Sept. 7, 2016, 5:24 a.m.
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 V1 -> V2:
- Add kfree exceptional handle to avoid memory leak.
- Improve code style.

---
 drivers/gpu/drm/drm_global.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
index 3d2e91c..b181e81 100644
--- a/drivers/gpu/drm/drm_global.c
+++ b/drivers/gpu/drm/drm_global.c
@@ -65,30 +65,33 @@  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 out;
 		}
-
-		ref->object = item->object;
 		ret = ref->init(ref);
 		if (unlikely(ret != 0))
 			goto out_err;
 
+		item->object = ref->object;
+	} else {
+		ref->object = item->object;
 	}
+
 	++item->refcount;
-	ref->object = item->object;
-	mutex_unlock(&item->mutex);
-	return 0;
+	goto out;
+
 out_err:
+	kfree(ref->object);
+	ref->object = NULL;
+out:
 	mutex_unlock(&item->mutex);
-	item->object = NULL;
 	return ret;
 }
 EXPORT_SYMBOL(drm_global_item_ref);

Comments

Am 07.09.2016 um 07:24 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>

Well when you update your patch, even when it's just fixing a small 
typo, please increase the version number.

That makes it much easier to track the different instances of a patch.

A few additional notes below.

> ---
>
> Changes from V1 -> V2:
> - Add kfree exceptional handle to avoid memory leak.
> - Improve code style.
>
> ---
>   drivers/gpu/drm/drm_global.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
> index 3d2e91c..b181e81 100644
> --- a/drivers/gpu/drm/drm_global.c
> +++ b/drivers/gpu/drm/drm_global.c
> @@ -65,30 +65,33 @@ 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 out;
>   		}
> -
> -		ref->object = item->object;
>   		ret = ref->init(ref);
>   		if (unlikely(ret != 0))
>   			goto out_err;
>   
> +		item->object = ref->object;
> +	} else {
> +		ref->object = item->object;
>   	}
> +
>   	++item->refcount;
> -	ref->object = item->object;
> -	mutex_unlock(&item->mutex);
> -	return 0;
> +	goto out;

A goto in the not error path is a bit unusual. Since out_err is only 
used once couldn't you just move the code into the if and then goto to 
out as well?

Alternative you could just duplicate the mutex release in the non-error 
path.

Regards,
Christian.

> +
>   out_err:
> +	kfree(ref->object);
> +	ref->object = NULL;
> +out:
>   	mutex_unlock(&item->mutex);
> -	item->object = NULL;
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_global_item_ref);
On Wed, Sep 07, 2016 at 09:12:29AM +0200, Christian König wrote:
> Am 07.09.2016 um 07:24 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>
> 
> Well when you update your patch, even when it's just fixing a small
> typo, please increase the version number.
> 
> That makes it much easier to track the different instances of a patch.
> 

OK.

> A few additional notes below.
> 
> >---
> >
> >Changes from V1 -> V2:
> >- Add kfree exceptional handle to avoid memory leak.
> >- Improve code style.
> >
> >---
> >  drivers/gpu/drm/drm_global.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
> >index 3d2e91c..b181e81 100644
> >--- a/drivers/gpu/drm/drm_global.c
> >+++ b/drivers/gpu/drm/drm_global.c
> >@@ -65,30 +65,33 @@ 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 out;
> >  		}
> >-
> >-		ref->object = item->object;
> >  		ret = ref->init(ref);
> >  		if (unlikely(ret != 0))
> >  			goto out_err;
> >+		item->object = ref->object;
> >+	} else {
> >+		ref->object = item->object;
> >  	}
> >+
> >  	++item->refcount;
> >-	ref->object = item->object;
> >-	mutex_unlock(&item->mutex);
> >-	return 0;
> >+	goto out;
> 
> A goto in the not error path is a bit unusual. Since out_err is only
> used once couldn't you just move the code into the if and then goto
> to out as well?
> 
> Alternative you could just duplicate the mutex release in the
> non-error path.
> 

Actually, I also have a little concern with "goto" in non-error path. But
as Sean's suggestion, use "goto" can avoid a duplicate mutex release, you
know.

@Sean, if you don't have concern with adding a mutex release below, I will
update it in V3.

---
        ++item->refcount;
        mutex_unlock(&item->mutex);
        return 0;

out_err:
        kfree(ref->object);
        ref->object = NULL;
out:
        mutex_unlock(&item->mutex);
        return ret;
---


Thanks,
Rui
Am 07.09.2016 um 09:54 schrieb Huang Rui:
> On Wed, Sep 07, 2016 at 09:12:29AM +0200, Christian König wrote:
>> Am 07.09.2016 um 07:24 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>
>> Well when you update your patch, even when it's just fixing a small
>> typo, please increase the version number.
>>
>> That makes it much easier to track the different instances of a patch.
>>
> OK.
>
>> A few additional notes below.
>>
>>> ---
>>>
>>> Changes from V1 -> V2:
>>> - Add kfree exceptional handle to avoid memory leak.
>>> - Improve code style.
>>>
>>> ---
>>>   drivers/gpu/drm/drm_global.c | 23 +++++++++++++----------
>>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
>>> index 3d2e91c..b181e81 100644
>>> --- a/drivers/gpu/drm/drm_global.c
>>> +++ b/drivers/gpu/drm/drm_global.c
>>> @@ -65,30 +65,33 @@ 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 out;
>>>   		}
>>> -
>>> -		ref->object = item->object;
>>>   		ret = ref->init(ref);
>>>   		if (unlikely(ret != 0))
>>>   			goto out_err;
>>> +		item->object = ref->object;
>>> +	} else {
>>> +		ref->object = item->object;
>>>   	}
>>> +
>>>   	++item->refcount;
>>> -	ref->object = item->object;
>>> -	mutex_unlock(&item->mutex);
>>> -	return 0;
>>> +	goto out;
>> A goto in the not error path is a bit unusual. Since out_err is only
>> used once couldn't you just move the code into the if and then goto
>> to out as well?
>>
>> Alternative you could just duplicate the mutex release in the
>> non-error path.
>>
> Actually, I also have a little concern with "goto" in non-error path. But
> as Sean's suggestion, use "goto" can avoid a duplicate mutex release, you
> know.

I would certainly prefer the duplicate mutex release.

>
> @Sean, if you don't have concern with adding a mutex release below, I will
> update it in V3.
>
> ---
>          ++item->refcount;
>          mutex_unlock(&item->mutex);
>          return 0;
>
> out_err:
>          kfree(ref->object);
>          ref->object = NULL;
> out:

BTW: I would name those labels error_free and error_unlock to make clear 
that they are for error handling and what they are supposed to do.

Regards,
Christian.

>          mutex_unlock(&item->mutex);
>          return ret;
> ---
>
>
> Thanks,
> Rui
On Wed, Sep 07, 2016 at 04:02:47PM +0800, Koenig, Christian wrote:
> Am 07.09.2016 um 09:54 schrieb Huang Rui:
> > On Wed, Sep 07, 2016 at 09:12:29AM +0200, Christian K?nig wrote:
> >> Am 07.09.2016 um 07:24 schrieb Huang Rui:

snip.

> >>>   	++item->refcount;
> >>> -	ref->object = item->object;
> >>> -	mutex_unlock(&item->mutex);
> >>> -	return 0;
> >>> +	goto out;
> >> A goto in the not error path is a bit unusual. Since out_err is only
> >> used once couldn't you just move the code into the if and then goto
> >> to out as well?
> >>
> >> Alternative you could just duplicate the mutex release in the
> >> non-error path.
> >>
> > Actually, I also have a little concern with "goto" in non-error path. But
> > as Sean's suggestion, use "goto" can avoid a duplicate mutex release, you
> > know.
> 
> I would certainly prefer the duplicate mutex release.
> 
> >
> > @Sean, if you don't have concern with adding a mutex release below, I will
> > update it in V3.
> >
> > ---
> >          ++item->refcount;
> >          mutex_unlock(&item->mutex);
> >          return 0;
> >
> > out_err:
> >          kfree(ref->object);
> >          ref->object = NULL;
> > out:
> 
> BTW: I would name those labels error_free and error_unlock to make clear 
> that they are for error handling and what they are supposed to do.
> 

OK, will rename the label in V3.

Thanks,
Rui