Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On Mon, 2014-08-25 at 09:35 -0500, Aaron Watry wrote:
> On Sat, Aug 23, 2014 at 10:01 AM, Bruno Jimenez wrote:
> > On Thu, 2014-08-21 at 14:37 -0500, Aaron Watry wrote:
> >> v2: Change to C-style comments and fix indentation
> >>
> >> Signed-off-by: Aaron Watry
> >> ---
> >> src/gallium/drivers/r600/compute_memory_pool.c | 5 +
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/src/gallium/drivers/r600/compute_memory_pool.c
> >> b/src/gallium/drivers/r600/compute_memory_pool.c
> >> index 9324b84..82de9cd 100644
> >> --- a/src/gallium/drivers/r600/compute_memory_pool.c
> >> +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> >> @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct
> >> compute_memory_pool* pool)
> >> pool->screen->b.b.resource_destroy((struct pipe_screen *)
> >> pool->screen, (struct pipe_resource *)pool->bo);
> >> }
> >> + /* In theory, all of the items were freed in compute_memory_free.
> >> +Just delete the list heads */
> >
> > Hi,
> >
> > If you are worried about the items not have been freed, you can try
> > doing something like this (mostly copied from compute_memory_free):
> >
>
> I'm not actually worried about these items having not been freed... At
> least up to this point, I
> haven't seen any pool items leaking in valgrind, just the list heads
> themselves.
>
> If you want, I can add this type of foreach cleanup, but I'm content
> to leave it as is until it becomes
> an issue... Your choice.
>
> --Aaron
Hi,
If Valgrind says that there doesn't seem to be any leak, then I'm fine
with the patch as is.
For this patch, with the comment changes suggested by Michel.
Reviewed-by: Bruno Jiménez
>
>
> > struct compute_memory_item *item, *next;
> > struct pipe_screen *screen = (struct pipe_screen *)pool->screen;
> > struct pipe_resource *res;
> >
> > if (!LIST_IS_EMPTY(pool->item_list)) {
> > LIST_FOR_EACH_ENTRY_SAFE(item, next, pool->item_list, link) {
> > if (item->real_buffer) {
> > res = (struct pipe_resource *)item->real_buffer;
> > pool->screen->b.b.resource_destroy(screen, res);
> > }
> >
> > free(item);
> > }
> > }
> > /* And the same for the unallocated_list */
> >
> > Note: I haven't tested it, but I think that it should work.
> >
> > Hope it helps!
> > Bruno
> >
> >> + free(pool->item_list);
> >> + free(pool->unallocated_list);
> >> + /* And then the pool itself */
> >> free(pool);
> >> }
> >>
> >
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On Mon, 2014-08-25 at 18:04 +0200, Jan Vesely wrote:
> On Sat, 2014-08-23 at 17:01 +0200, Bruno Jimenez wrote:
> > On Thu, 2014-08-21 at 14:37 -0500, Aaron Watry wrote:
> > > v2: Change to C-style comments and fix indentation
> > >
> > > Signed-off-by: Aaron Watry
> > > ---
> > > src/gallium/drivers/r600/compute_memory_pool.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/src/gallium/drivers/r600/compute_memory_pool.c
> > > b/src/gallium/drivers/r600/compute_memory_pool.c
> > > index 9324b84..82de9cd 100644
> > > --- a/src/gallium/drivers/r600/compute_memory_pool.c
> > > +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> > > @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct
> > > compute_memory_pool* pool)
> > > pool->screen->b.b.resource_destroy((struct pipe_screen *)
> > > pool->screen, (struct pipe_resource *)pool->bo);
> > > }
> > > + /* In theory, all of the items were freed in compute_memory_free.
> > > +Just delete the list heads */
> >
> > Hi,
> >
> > If you are worried about the items not have been freed, you can try
> > doing something like this (mostly copied from compute_memory_free):
>
> Is the situation legal? My current understanding is that leftover items
> would indicate a bug somewhere in the state tracker.
> would assert(LIST_IS_EMPTY(pool->item_list))) ever hit?
>
> jan
Hi,
I'm afraid that my knowledge of the state tracker is not enough to
assure this. But I mostly agree that the state tracker should be
responsible for deleting all the items before deleting the pool itself.
Anyway, Aaron says that there doesn't seem to be any item leaking,
according to valgrind. So I'm perfectly fine by just freeing the list
heads.
Bruno
>
> >
> > struct compute_memory_item *item, *next;
> > struct pipe_screen *screen = (struct pipe_screen *)pool->screen;
> > struct pipe_resource *res;
> >
> > if (!LIST_IS_EMPTY(pool->item_list)) {
> > LIST_FOR_EACH_ENTRY_SAFE(item, next, pool->item_list, link) {
> > if (item->real_buffer) {
> > res = (struct pipe_resource *)item->real_buffer;
> > pool->screen->b.b.resource_destroy(screen, res);
> > }
> >
> > free(item);
> > }
> > }
> > /* And the same for the unallocated_list */
> >
> > Note: I haven't tested it, but I think that it should work.
> >
> > Hope it helps!
> > Bruno
> >
> > > + free(pool->item_list);
> > > + free(pool->unallocated_list);
> > > + /* And then the pool itself */
> > > free(pool);
> > > }
> > >
> >
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On Sat, 2014-08-23 at 17:01 +0200, Bruno Jimenez wrote:
> On Thu, 2014-08-21 at 14:37 -0500, Aaron Watry wrote:
> > v2: Change to C-style comments and fix indentation
> >
> > Signed-off-by: Aaron Watry
> > ---
> > src/gallium/drivers/r600/compute_memory_pool.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/src/gallium/drivers/r600/compute_memory_pool.c
> > b/src/gallium/drivers/r600/compute_memory_pool.c
> > index 9324b84..82de9cd 100644
> > --- a/src/gallium/drivers/r600/compute_memory_pool.c
> > +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> > @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct
> > compute_memory_pool* pool)
> > pool->screen->b.b.resource_destroy((struct pipe_screen *)
> > pool->screen, (struct pipe_resource *)pool->bo);
> > }
> > + /* In theory, all of the items were freed in compute_memory_free.
> > + Just delete the list heads */
>
> Hi,
>
> If you are worried about the items not have been freed, you can try
> doing something like this (mostly copied from compute_memory_free):
Is the situation legal? My current understanding is that leftover items
would indicate a bug somewhere in the state tracker.
would assert(LIST_IS_EMPTY(pool->item_list))) ever hit?
jan
>
> struct compute_memory_item *item, *next;
> struct pipe_screen *screen = (struct pipe_screen *)pool->screen;
> struct pipe_resource *res;
>
> if (!LIST_IS_EMPTY(pool->item_list)) {
> LIST_FOR_EACH_ENTRY_SAFE(item, next, pool->item_list, link) {
> if (item->real_buffer) {
> res = (struct pipe_resource *)item->real_buffer;
> pool->screen->b.b.resource_destroy(screen, res);
> }
>
> free(item);
> }
> }
> /* And the same for the unallocated_list */
>
> Note: I haven't tested it, but I think that it should work.
>
> Hope it helps!
> Bruno
>
> > + free(pool->item_list);
> > + free(pool->unallocated_list);
> > + /* And then the pool itself */
> > free(pool);
> > }
> >
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Jan Vesely
signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On Sat, Aug 23, 2014 at 10:01 AM, Bruno Jimenez wrote:
> On Thu, 2014-08-21 at 14:37 -0500, Aaron Watry wrote:
>> v2: Change to C-style comments and fix indentation
>>
>> Signed-off-by: Aaron Watry
>> ---
>> src/gallium/drivers/r600/compute_memory_pool.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/src/gallium/drivers/r600/compute_memory_pool.c
>> b/src/gallium/drivers/r600/compute_memory_pool.c
>> index 9324b84..82de9cd 100644
>> --- a/src/gallium/drivers/r600/compute_memory_pool.c
>> +++ b/src/gallium/drivers/r600/compute_memory_pool.c
>> @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct
>> compute_memory_pool* pool)
>> pool->screen->b.b.resource_destroy((struct pipe_screen *)
>> pool->screen, (struct pipe_resource *)pool->bo);
>> }
>> + /* In theory, all of the items were freed in compute_memory_free.
>> +Just delete the list heads */
>
> Hi,
>
> If you are worried about the items not have been freed, you can try
> doing something like this (mostly copied from compute_memory_free):
>
I'm not actually worried about these items having not been freed... At
least up to this point, I
haven't seen any pool items leaking in valgrind, just the list heads themselves.
If you want, I can add this type of foreach cleanup, but I'm content
to leave it as is until it becomes
an issue... Your choice.
--Aaron
> struct compute_memory_item *item, *next;
> struct pipe_screen *screen = (struct pipe_screen *)pool->screen;
> struct pipe_resource *res;
>
> if (!LIST_IS_EMPTY(pool->item_list)) {
> LIST_FOR_EACH_ENTRY_SAFE(item, next, pool->item_list, link) {
> if (item->real_buffer) {
> res = (struct pipe_resource *)item->real_buffer;
> pool->screen->b.b.resource_destroy(screen, res);
> }
>
> free(item);
> }
> }
> /* And the same for the unallocated_list */
>
> Note: I haven't tested it, but I think that it should work.
>
> Hope it helps!
> Bruno
>
>> + free(pool->item_list);
>> + free(pool->unallocated_list);
>> + /* And then the pool itself */
>> free(pool);
>> }
>>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On 22.08.2014 04:37, Aaron Watry wrote: v2: Change to C-style comments and fix indentation Signed-off-by: Aaron Watry --- src/gallium/drivers/r600/compute_memory_pool.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/gallium/drivers/r600/compute_memory_pool.c b/src/gallium/drivers/r600/compute_memory_pool.c index 9324b84..82de9cd 100644 --- a/src/gallium/drivers/r600/compute_memory_pool.c +++ b/src/gallium/drivers/r600/compute_memory_pool.c @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct compute_memory_pool* pool) pool->screen->b.b.resource_destroy((struct pipe_screen *) pool->screen, (struct pipe_resource *)pool->bo); } + /* In theory, all of the items were freed in compute_memory_free. + Just delete the list heads */ We generally use this format for multi-line comments in our driver code: /* First line * [...] * Last line */ -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
On Thu, 2014-08-21 at 14:37 -0500, Aaron Watry wrote:
> v2: Change to C-style comments and fix indentation
>
> Signed-off-by: Aaron Watry
> ---
> src/gallium/drivers/r600/compute_memory_pool.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/gallium/drivers/r600/compute_memory_pool.c
> b/src/gallium/drivers/r600/compute_memory_pool.c
> index 9324b84..82de9cd 100644
> --- a/src/gallium/drivers/r600/compute_memory_pool.c
> +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct
> compute_memory_pool* pool)
> pool->screen->b.b.resource_destroy((struct pipe_screen *)
> pool->screen, (struct pipe_resource *)pool->bo);
> }
> + /* In theory, all of the items were freed in compute_memory_free.
> +Just delete the list heads */
Hi,
If you are worried about the items not have been freed, you can try
doing something like this (mostly copied from compute_memory_free):
struct compute_memory_item *item, *next;
struct pipe_screen *screen = (struct pipe_screen *)pool->screen;
struct pipe_resource *res;
if (!LIST_IS_EMPTY(pool->item_list)) {
LIST_FOR_EACH_ENTRY_SAFE(item, next, pool->item_list, link) {
if (item->real_buffer) {
res = (struct pipe_resource *)item->real_buffer;
pool->screen->b.b.resource_destroy(screen, res);
}
free(item);
}
}
/* And the same for the unallocated_list */
Note: I haven't tested it, but I think that it should work.
Hope it helps!
Bruno
> + free(pool->item_list);
> + free(pool->unallocated_list);
> + /* And then the pool itself */
> free(pool);
> }
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] r600/compute: Don't leak compute pool item_list/unallocated_list
v2: Change to C-style comments and fix indentation Signed-off-by: Aaron Watry --- src/gallium/drivers/r600/compute_memory_pool.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/gallium/drivers/r600/compute_memory_pool.c b/src/gallium/drivers/r600/compute_memory_pool.c index 9324b84..82de9cd 100644 --- a/src/gallium/drivers/r600/compute_memory_pool.c +++ b/src/gallium/drivers/r600/compute_memory_pool.c @@ -95,6 +95,11 @@ void compute_memory_pool_delete(struct compute_memory_pool* pool) pool->screen->b.b.resource_destroy((struct pipe_screen *) pool->screen, (struct pipe_resource *)pool->bo); } + /* In theory, all of the items were freed in compute_memory_free. + Just delete the list heads */ + free(pool->item_list); + free(pool->unallocated_list); + /* And then the pool itself */ free(pool); } -- 1.9.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
