| Message ID | 20170629134901.21909-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/06/2017 14:49, Chris Wilson wrote: > When computing a hash for looking up relcoation target handles in an > execbuf, we start with a large size for the hashtable and proceed to > reduce it until the allocation suceeds. The final attempt is with an > order of 0 (i.e. a single element). This means that we then pass bits=0 > to hash_32() which then computes "hash >> (32 - 0)" to lookup the single > element. Right shifting by a value the width of the operand is > undefined, so limit the smallest hash table we use to order 1. > > Fixes: 4ff4b44cbb70 ("drm/i915: Store a direct lookup from object handle to vma") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 718bb75ad387..54791bcb8ccb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -296,12 +296,8 @@ static int eb_create(struct i915_execbuffer *eb) > break; > } while (--size); > > - if (unlikely(!eb->buckets)) { > - eb->buckets = kzalloc(sizeof(struct hlist_head), > - GFP_TEMPORARY); Want to maybe drop the NORETRY | NOWARN from the remaining allocation now? Wasn't it recently discussed that it is to feeble in it's attempts to allocate? > - if (unlikely(!eb->buckets)) > - return -ENOMEM; > - } > + if (unlikely(!eb->buckets)) > + return -ENOMEM; > > eb->lut_size = size; > } else { > @@ -453,7 +449,7 @@ eb_add_vma(struct i915_execbuffer *eb, > return err; > } > > - if (eb->lut_size >= 0) { > + if (eb->lut_size > 0) { > vma->exec_handle = entry->handle; > hlist_add_head(&vma->exec_node, > &eb->buckets[hash_32(entry->handle, > @@ -897,7 +893,7 @@ static void eb_release_vmas(const struct i915_execbuffer *eb) > static void eb_reset_vmas(const struct i915_execbuffer *eb) > { > eb_release_vmas(eb); > - if (eb->lut_size >= 0) > + if (eb->lut_size > 0) > memset(eb->buckets, 0, > sizeof(struct hlist_head) << eb->lut_size); > } > @@ -906,7 +902,7 @@ static void eb_destroy(const struct i915_execbuffer *eb) > { > GEM_BUG_ON(eb->reloc_cache.rq); > > - if (eb->lut_size >= 0) > + if (eb->lut_size > 0) > kfree(eb->buckets); > } > > @@ -2185,8 +2181,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, > } > } > > - if (eb_create(&eb)) > - return -ENOMEM; > + err = eb_create(&eb); > + if (err) > + goto err_out_fence; > + > + GEM_BUG_ON(!eb.lut_size); > > err = eb_select_context(&eb); > if (unlikely(err)) > @@ -2346,6 +2345,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, > i915_gem_context_put(eb.ctx); > err_destroy: > eb_destroy(&eb); > +err_out_fence: > if (out_fence_fd != -1) > put_unused_fd(out_fence_fd); > err_in_fence: > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2017-06-29 15:50:17) > > On 29/06/2017 14:49, Chris Wilson wrote: > > When computing a hash for looking up relcoation target handles in an > > execbuf, we start with a large size for the hashtable and proceed to > > reduce it until the allocation suceeds. The final attempt is with an > > order of 0 (i.e. a single element). This means that we then pass bits=0 > > to hash_32() which then computes "hash >> (32 - 0)" to lookup the single > > element. Right shifting by a value the width of the operand is > > undefined, so limit the smallest hash table we use to order 1. > > > > Fixes: 4ff4b44cbb70 ("drm/i915: Store a direct lookup from object handle to vma") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 718bb75ad387..54791bcb8ccb 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -296,12 +296,8 @@ static int eb_create(struct i915_execbuffer *eb) > > break; > > } while (--size); > > > > - if (unlikely(!eb->buckets)) { > > - eb->buckets = kzalloc(sizeof(struct hlist_head), > > - GFP_TEMPORARY); > > Want to maybe drop the NORETRY | NOWARN from the remaining allocation > now? Wasn't it recently discussed that it is to feeble in it's attempts > to allocate? True that was the point of the allocate afterwards, so that we could do it with the retry flags. Time for a respin. -Chris
Quoting Patchwork (2017-06-29 16:27:13) > == Series Details == > > Series: drm/i915: Avoid undefined behaviour of "u32 >> 32" (rev2) > URL : https://patchwork.freedesktop.org/series/26556/ > State : success > > == Summary == > > Series 26556v2 drm/i915: Avoid undefined behaviour of "u32 >> 32" > https://patchwork.freedesktop.org/api/1.0/series/26556/revisions/2/mbox/ > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-uc: > pass -> FAIL (fi-snb-2600) fdo#100007 > Test gem_exec_suspend: > Subgroup basic-s4-devices: > dmesg-warn -> PASS (fi-kbl-r) fdo#100125 > Test kms_cursor_legacy: > Subgroup basic-busy-flip-before-cursor-atomic: > fail -> PASS (fi-snb-2600) fdo#100215 > Test kms_pipe_crc_basic: > Subgroup hang-read-crc-pipe-a: > dmesg-warn -> PASS (fi-pnv-d510) fdo#101597 +1 > > fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 > fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 > fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215 > fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597 Hopefully this explains the weird overrun of eb->buckets[] Tomi reported in eb_add_vma() under tight memory conditions. Pushed, thanks for the review, -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 718bb75ad387..54791bcb8ccb 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -296,12 +296,8 @@ static int eb_create(struct i915_execbuffer *eb) break; } while (--size); - if (unlikely(!eb->buckets)) { - eb->buckets = kzalloc(sizeof(struct hlist_head), - GFP_TEMPORARY); - if (unlikely(!eb->buckets)) - return -ENOMEM; - } + if (unlikely(!eb->buckets)) + return -ENOMEM; eb->lut_size = size; } else { @@ -453,7 +449,7 @@ eb_add_vma(struct i915_execbuffer *eb, return err; } - if (eb->lut_size >= 0) { + if (eb->lut_size > 0) { vma->exec_handle = entry->handle; hlist_add_head(&vma->exec_node, &eb->buckets[hash_32(entry->handle, @@ -897,7 +893,7 @@ static void eb_release_vmas(const struct i915_execbuffer *eb) static void eb_reset_vmas(const struct i915_execbuffer *eb) { eb_release_vmas(eb); - if (eb->lut_size >= 0) + if (eb->lut_size > 0) memset(eb->buckets, 0, sizeof(struct hlist_head) << eb->lut_size); } @@ -906,7 +902,7 @@ static void eb_destroy(const struct i915_execbuffer *eb) { GEM_BUG_ON(eb->reloc_cache.rq); - if (eb->lut_size >= 0) + if (eb->lut_size > 0) kfree(eb->buckets); } @@ -2185,8 +2181,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, } } - if (eb_create(&eb)) - return -ENOMEM; + err = eb_create(&eb); + if (err) + goto err_out_fence; + + GEM_BUG_ON(!eb.lut_size); err = eb_select_context(&eb); if (unlikely(err)) @@ -2346,6 +2345,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, i915_gem_context_put(eb.ctx); err_destroy: eb_destroy(&eb); +err_out_fence: if (out_fence_fd != -1) put_unused_fd(out_fence_fd); err_in_fence:
When computing a hash for looking up relcoation target handles in an execbuf, we start with a large size for the hashtable and proceed to reduce it until the allocation suceeds. The final attempt is with an order of 0 (i.e. a single element). This means that we then pass bits=0 to hash_32() which then computes "hash >> (32 - 0)" to lookup the single element. Right shifting by a value the width of the operand is undefined, so limit the smallest hash table we use to order 1. Fixes: 4ff4b44cbb70 ("drm/i915: Store a direct lookup from object handle to vma") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)