[Mesa-dev] [PATCH 1/2] r600: Fix possible endless loop in compute_memory_pool allocations.

[Bruno Jimenez]

On Thu, 2014-06-19 at 11:58 -0400, Jan Vesely wrote:
> On Thu, 2014-06-19 at 08:27 -0700, Tom Stellard wrote:
> > On Thu, Jun 19, 2014 at 10:21:32AM -0400, Jan Vesely wrote:
> > > The important part is the change of the condition to <= 0. Otherwise the loop
> > > gets stuck never actually growing the pool.
> > > 
> > > The change in the aux-need calculation guarantees max 2 iterations, and
> > > avoids wasting memory in case a smaller item can't fit into a relatively larger
> > > pool.
> > >
> > 
> > Does this patch obsolete the XXX comment around line 292 of this file?  If so,
> > we should remove it.
> 
> I'm not sure if the XXX comment applies to the first:
> if (pool->size_in_dw < allocated+unallocated)
> or in general.
> In general I think the comment is obsolete as is. There already is a
> logic that tries to grow the pool if allocation fails despite enough
> free space. The patch only changes the the condition to include free
> space==needed space corner case.
> The change in requested size is separate and does not change the
> situation.

Hi,

As far as I understand it, this patch doesn't make obsolete that
comment. But that may be because I understant that that comment only
refers to the first grow of the pool, and the rest is a workaround to
continue growing the pool if we can't fit in the new items.

> 
> > 
> > Also have tried this with patches 1-9 of this series:
> > http://lists.freedesktop.org/archives/mesa-dev/2014-June/061742.html
> 
> I have not tried applying the patches, but patch 5/11 moves the very
> same logic ( if (need <0)), to a slightly different location. So I think
> that patch needs to be updated to include the fix.

You are right, this patch is still needed in my series.

> That series also removes one call to compute_memory_pool_finalize, so
> adding a check there now might be redundant. However, I'd still prefer
> to check return value of both calls to compute_memory_pool_finalize for
> consistency.

I'm OK with adding both calls, and I don't think it is redundant.
Imagine the case where my series don't land, we would have one call
checked and the other not.

I'll try to rebase my series on top of yours.

Thanks!
Bruno

> 
> 
> regards,
> Jan
> 
> > 
> > -Tom
> >  
> > > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> > > CC: Bruno Jimenez <brunojimen at gmail.com>
> > > ---
> > > 
> > > This fixes hang in gegl colors.xml test
> > > 
> > >  src/gallium/drivers/r600/compute_memory_pool.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/gallium/drivers/r600/compute_memory_pool.c b/src/gallium/drivers/r600/compute_memory_pool.c
> > > index ec8c470..0b6d2da6 100644
> > > --- a/src/gallium/drivers/r600/compute_memory_pool.c
> > > +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> > > @@ -320,8 +320,11 @@ int compute_memory_finalize_pending(struct compute_memory_pool* pool,
> > >  			int64_t need = item->size_in_dw+2048 -
> > >  						(pool->size_in_dw - allocated);
> > >  
> > > -			if (need < 0) {
> > > -				need = pool->size_in_dw / 10;
> > > +			if (need <= 0) {
> > > +				/* There's enough free space, but it's too
> > > +				 * fragmented. Assume half of the item can fit
> > > +				 * int the last chunk */
> > > +				need = (item->size_in_dw / 2) + ITEM_ALIGNMENT;
> > >  			}
> > >  
> > >  			need = align(need, ITEM_ALIGNMENT);
> > > -- 
> > > 1.9.3
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>