| Message ID | 20190704104345.6603-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gtt: Handle double alloc failures | expand |
On Thu, 4 Jul 2019 at 11:43, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Matthew pointed out that we could face a double failure with concurrent > allocations/frees, and so the assumption that the local var alloc was > NULL was fraught with danger. Rather than complicate the error paths too > much to add a second local for a second free, just do the second free > earlier on the unwind path. > > Reported-by: Matthew Auld <matthew.william.auld@gmail.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.william.auld@gmail.com> I quite liked your previous pattern: @@ -1442,6 +1442,7 @@ static int gen8_ppgtt_alloc_pdp(struct i915_address_space *vm, { struct i915_page_directory *pd, *alloc = NULL; u64 from = start; + bool free = false; unsigned int pdpe; int ret = 0; @@ -1489,10 +1490,11 @@ static int gen8_ppgtt_alloc_pdp(struct i915_address_space *vm, gen8_ppgtt_set_pdpe(pdp, vm->scratch_pd, pdpe); GEM_BUG_ON(!atomic_read(&pdp->used)); atomic_dec(&pdp->used); - GEM_BUG_ON(alloc); - alloc = pd; /* defer the free to after the lock */ + free = true; } spin_unlock(&pdp->lock); + if (free) + free_pd(vm, pd); unwind: gen8_ppgtt_clear_pdp(vm, pdp, from, start - from); out: Anyway, Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
Quoting Matthew Auld (2019-07-04 13:27:07) > On Thu, 4 Jul 2019 at 11:43, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Matthew pointed out that we could face a double failure with concurrent > > allocations/frees, and so the assumption that the local var alloc was > > NULL was fraught with danger. Rather than complicate the error paths too > > much to add a second local for a second free, just do the second free > > earlier on the unwind path. > > > > Reported-by: Matthew Auld <matthew.william.auld@gmail.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > I quite liked your previous pattern: > > @@ -1442,6 +1442,7 @@ static int gen8_ppgtt_alloc_pdp(struct > i915_address_space *vm, > { > struct i915_page_directory *pd, *alloc = NULL; > u64 from = start; > + bool free = false; > unsigned int pdpe; > int ret = 0; > > @@ -1489,10 +1490,11 @@ static int gen8_ppgtt_alloc_pdp(struct > i915_address_space *vm, > gen8_ppgtt_set_pdpe(pdp, vm->scratch_pd, pdpe); > GEM_BUG_ON(!atomic_read(&pdp->used)); > atomic_dec(&pdp->used); > - GEM_BUG_ON(alloc); > - alloc = pd; /* defer the free to after the lock */ > + free = true; > } > spin_unlock(&pdp->lock); > + if (free) > + free_pd(vm, pd); > unwind: > gen8_ppgtt_clear_pdp(vm, pdp, from, start - from); > out: > > Anyway, > Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com> I'm sure Mika is dying to rewrite these anyway, we can see what solution he comes up with. :) -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 1065753e86fb..9756f1b670e9 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1484,6 +1484,10 @@ static int gen8_ppgtt_alloc_pdp(struct i915_address_space *vm, goto out; unwind_pd: + if (alloc) { + free_pd(vm, alloc); + alloc = NULL; + } spin_lock(&pdp->lock); if (atomic_dec_and_test(&pd->used)) { gen8_ppgtt_set_pdpe(pdp, vm->scratch_pd, pdpe); @@ -1556,6 +1560,10 @@ static int gen8_ppgtt_alloc_4lvl(struct i915_address_space *vm, goto out; unwind_pdp: + if (alloc) { + free_pd(vm, alloc); + alloc = NULL; + } spin_lock(&pml4->lock); if (atomic_dec_and_test(&pdp->used)) { gen8_ppgtt_set_pml4e(pml4, vm->scratch_pdp, pml4e);
Matthew pointed out that we could face a double failure with concurrent allocations/frees, and so the assumption that the local var alloc was NULL was fraught with danger. Rather than complicate the error paths too much to add a second local for a second free, just do the second free earlier on the unwind path. Reported-by: Matthew Auld <matthew.william.auld@gmail.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.william.auld@gmail.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++++ 1 file changed, 8 insertions(+)