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

[Jan Vesely]

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.

> 
> 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.

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.


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

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140619/ced1a075/attachment-0001.sig>