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

[Jan Vesely]

On Thu, 2014-06-19 at 18:22 +0200, Bruno Jimenez wrote:
> 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.

Hi,

thanks for clarification. Since the pool is potentially grown to fit
every item, is the initial

compute_memory_grow_pool(pool, pipe, allocated+unallocated);

needed at all?

regards,
Jan

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

-- 
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/f1467035/attachment.sig>