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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
