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

[Daniel Vetter <daniel@ffwll.ch>]

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

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