Re: [PATCH 4/9] drm/i915: Limit the number of node allocation retries

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, May 07, 2014 at 09:49:57AM +0200, Daniel Vetter wrote:
> On Tue, May 06, 2014 at 10:21:33PM -0700, Ben Widawsky wrote:
> > AFAICT, it's impossible to actually infinitely retry the allocation in
> > our current code. However, a small oversight on my part, slight bug, or
> > future bug, could easily change that.
> > 
> > To avoid a potential breakage, a simple retry variable of 16 bits will
> > help us prevent infinitely running.
> > 
> > Retry is limited to 100 as a mostly random number. Some consideration
> > about stack usage was taken into account. As an example, if we allowed
> > 256 retries on a 32b arch (and my memory serves that all arguments are
> > passed on the stack for such architectures), thats 33 bytes * 256
> > retries + (fudge for return address and such)... it's way more than we
> > want to use already. 64b architectures might be slightly better, since
> > 6? of the first args will get passed through registers, but it's still
> > bad.
> > 
> > If anything, we might want to do way less than 100, like 3.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> So you replace the retry loop with a tailrecursive version in patch 2 and
> then add duct-tape later on here? Nope.
> -Daniel
> 

I feel like you're implying the retry loop will end, ever. The retry
timeout should probably come first though, I Agree with that much.

> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index b6965a2..de98b26 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3216,6 +3216,7 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
> >  #endif
> >  }
> >  
> > +#define MAX_VMA_FIND_RETRY 100
> >  static int
> >  i915_gem_find_vm_space(struct i915_address_space *vm,
> >  		       struct drm_mm_node *node,
> > @@ -3224,7 +3225,8 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
> >  		       unsigned long color,
> >  		       unsigned long start,
> >  		       unsigned long end,
> > -		       uint32_t flags)
> > +		       uint32_t flags,
> > +		       uint8_t retry)
> >  {
> >  	int ret;
> >  	ret = drm_mm_insert_node_in_range_generic(&vm->mm, node,
> > @@ -3232,7 +3234,7 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
> >  						  start, end,
> >  						  DRM_MM_SEARCH_DEFAULT,
> >  						  DRM_MM_CREATE_DEFAULT);
> > -	if (ret) {
> > +	if (ret && (retry < MAX_VMA_FIND_RETRY)) {
> >  		if (WARN_ON(ret != -ENOSPC))
> >  			return ret;
> >  
> > @@ -3241,7 +3243,8 @@ i915_gem_find_vm_space(struct i915_address_space *vm,
> >  		if (ret == 0)
> >  			return i915_gem_find_vm_space(vm, node,
> >  						      size, align, color,
> > -						      start, end, flags);
> > +						      start, end, flags,
> > +						      retry++);
> >  	}
> >  
> >  	return ret;
> > @@ -3306,8 +3309,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  	if (IS_ERR(vma))
> >  		goto err_unpin;
> >  
> > -	ret = i915_gem_find_vm_space(vm, &vma->node, size, alignment,
> > -				     obj->cache_level, 0, gtt_max, flags);
> > +	ret = i915_gem_find_vm_space(vm, &vma->node,
> > +				     size, alignment, obj->cache_level,
> > +				     0, gtt_max, flags, 1);
> >  	if (ret)
> >  		goto err_free_vma;
> >  
> > -- 
> > 1.9.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center