* [PATCH] drm: modify drm_global_item_ref to avoid two times of writing ref->object
@ 2016-09-05 7:00 Huang Rui
2016-09-06 14:33 ` Sean Paul
0 siblings, 1 reply; 3+ messages in thread
From: Huang Rui @ 2016-09-05 7:00 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Alex Deucher,
Dave Airlie, Christian König, Daniel Vetter
Cc: Ken Wang, Huang Rui, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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>
---
drivers/gpu/drm/drm_global.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
index 3d2e91c..3abe738 100644
--- a/drivers/gpu/drm/drm_global.c
+++ b/drivers/gpu/drm/drm_global.c
@@ -70,25 +70,25 @@ int drm_global_item_ref(struct drm_global_reference *ref)
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;
}
-
- 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;
out_err:
mutex_unlock(&item->mutex);
- item->object = NULL;
+ ref->object = NULL;
return ret;
}
EXPORT_SYMBOL(drm_global_item_ref);
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm: modify drm_global_item_ref to avoid two times of writing ref->object
2016-09-05 7:00 [PATCH] drm: modify drm_global_item_ref to avoid two times of writing ref->object Huang Rui
@ 2016-09-06 14:33 ` Sean Paul
2016-09-07 2:40 ` Huang Rui
0 siblings, 1 reply; 3+ messages in thread
From: Sean Paul @ 2016-09-06 14:33 UTC (permalink / raw)
To: Huang Rui
Cc: Daniel Vetter, dri-devel, amd-gfx, Alex Deucher, Ken Wang,
Christian König
On Mon, Sep 5, 2016 at 3:00 AM, Huang Rui <ray.huang@amd.com> 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>
> ---
> drivers/gpu/drm/drm_global.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
> index 3d2e91c..3abe738 100644
> --- a/drivers/gpu/drm/drm_global.c
> +++ b/drivers/gpu/drm/drm_global.c
> @@ -70,25 +70,25 @@ int drm_global_item_ref(struct drm_global_reference *ref)
>
> 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;
> }
> -
> - ref->object = item->object;
> ret = ref->init(ref);
> if (unlikely(ret != 0))
> goto out_err;
>
> - }
> + item->object = ref->object;
> + } else
> + ref->object = item->object;
> +
You should add the braces to all blocks if they're present in any block.
> ++item->refcount;
> - ref->object = item->object;
> mutex_unlock(&item->mutex);
> return 0;
> out_err:
> mutex_unlock(&item->mutex);
> - item->object = NULL;
> + ref->object = NULL;
It seems like you're leaking memory here. Do you need a
kfree(ref->object) before this?
I think perhaps you can simplify (or maybe streamline is a better
word) things better by unifying the unlock and return. Something like:
diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
index 3d2e91c..5b255e4 100644
--- a/drivers/gpu/drm/drm_global.c
+++ b/drivers/gpu/drm/drm_global.c
@@ -65,30 +65,31 @@ 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;
-
+ } else {
+ ref->object = item->object;
}
- ++item->refcount;
- ref->object = item->object;
- mutex_unlock(&item->mutex);
- return 0;
+
+ item->refcount++;
+ 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);
> return ret;
> }
> EXPORT_SYMBOL(drm_global_item_ref);
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm: modify drm_global_item_ref to avoid two times of writing ref->object
2016-09-06 14:33 ` Sean Paul
@ 2016-09-07 2:40 ` Huang Rui
0 siblings, 0 replies; 3+ messages in thread
From: Huang Rui @ 2016-09-07 2:40 UTC (permalink / raw)
To: Sean Paul
Cc: Daniel Vetter, dri-devel, amd-gfx, Deucher, Alexander, Wang, Ken,
Koenig, Christian
On Tue, Sep 06, 2016 at 10:33:18PM +0800, Sean Paul wrote:
> On Mon, Sep 5, 2016 at 3:00 AM, Huang Rui <ray.huang@amd.com> 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>
> > ---
> > drivers/gpu/drm/drm_global.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
> > index 3d2e91c..3abe738 100644
> > --- a/drivers/gpu/drm/drm_global.c
> > +++ b/drivers/gpu/drm/drm_global.c
> > @@ -70,25 +70,25 @@ int drm_global_item_ref(struct drm_global_reference *ref)
> >
> > 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;
> > }
> > -
> > - ref->object = item->object;
> > ret = ref->init(ref);
> > if (unlikely(ret != 0))
> > goto out_err;
> >
> > - }
> > + item->object = ref->object;
> > + } else
> > + ref->object = item->object;
> > +
>
> You should add the braces to all blocks if they're present in any block.
>
> > ++item->refcount;
> > - ref->object = item->object;
> > mutex_unlock(&item->mutex);
> > return 0;
> > out_err:
> > mutex_unlock(&item->mutex);
> > - item->object = NULL;
> > + ref->object = NULL;
>
> It seems like you're leaking memory here. Do you need a
> kfree(ref->object) before this?
Yep, right. The original code seems to missed a kfree exceptional handle.
>
> I think perhaps you can simplify (or maybe streamline is a better
> word) things better by unifying the unlock and return. Something like:
>
Thanks, will update it in V2.
Rui
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-07 2:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-05 7:00 [PATCH] drm: modify drm_global_item_ref to avoid two times of writing ref->object Huang Rui
2016-09-06 14:33 ` Sean Paul
2016-09-07 2:40 ` Huang Rui
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.