From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/7] drm/i915/gem: Don't leak non-persistent requests on changing engines
Date: Tue, 11 Feb 2020 14:15:57 +0000 [thread overview]
Message-ID: <158143055793.2303.1572726675050076513@skylake-alporthouse-com> (raw)
In-Reply-To: <5e404774-1e4b-1c8a-f5ca-45826d7bda86@linux.intel.com>
Quoting Tvrtko Ursulin (2020-02-11 13:41:22)
>
> On 10/02/2020 20:57, Chris Wilson wrote:
> > +static void kill_context(struct i915_gem_context *ctx)
> > +{
> > + if (!list_empty(&ctx->stale.engines))
> > + kill_stale_engines(ctx);
>
> Lets see.. set_engines can freely race with context_close. The former is
> adding entries to the list under the lock, the latter is here inspecting
> list state unlocked. But then proceeds to lock it and all is good if
> false negative are not an issue. But looks like it could happen and then
> it fails to clean up. All that is needed is for this thread to not see
> the addition to the list. And since this is not a hot path how about you
> just always call kill_state_engines?
Hmm. I didn't consider the race between close context and set-engines.
We would also need to reject the late addition of engines to a closed
context under the spinlock.
Ta.
> > #endif /* __I915_GEM_CONTEXT_TYPES_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> > index 51ba97daf2a0..bc6d4f8b78f0 100644
> > --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> > @@ -211,10 +211,23 @@ void i915_sw_fence_complete(struct i915_sw_fence *fence)
> > __i915_sw_fence_complete(fence, NULL);
> > }
> >
> > -void i915_sw_fence_await(struct i915_sw_fence *fence)
> > +bool i915_sw_fence_await(struct i915_sw_fence *fence)
> > {
> > - debug_fence_assert(fence);
> > - WARN_ON(atomic_inc_return(&fence->pending) <= 1);
> > + int old, new;
> > +
> > + /*
> > + * It is only safe to add a new await to the fence while it has
> > + * not yet been signaled.
> > + */
> > + new = atomic_read(&fence->pending);
> > + do {
> > + if (new < 1)
> > + return false;
> > +
> > + old = new++;
> > + } while ((new = atomic_cmpxchg(&fence->pending, old, new)) != old);
>
> Simplify with atomic_try_cmpxchg?
I was under the mistaken impression we didn't have atomic_try_cmpxchg.
> I need a refresher on sw_fence->pending. (See your new comments and
> raise you lack of old! ;)
>
> -1 = completed
> 0 = ??
-1 = completed (all listeners awoken)
0 = signaled
1+ = pending waits (and yes we always start with 1 pending wait on behalf
of the caller)
> 1 = new, not waited upon
> 2 = waited upon
Maybe we don't really need -1? That was originally to avoid passing
FENCE_COMPLETE, FENCE_FREE but since we have the state, we don't need to
encode it? That would lead to a few small simplifications.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-02-11 14:16 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-10 20:57 [Intel-gfx] [PATCH 1/7] drm/i915/gt: Avoid resetting ring->head outside of its timeline mutex Chris Wilson
2020-02-10 20:57 ` [Intel-gfx] [PATCH 2/7] drm/i915/selftests: Exercise timeslice rewinding Chris Wilson
2020-02-11 14:50 ` Mika Kuoppala
2020-02-11 15:16 ` Chris Wilson
2020-02-10 20:57 ` [Intel-gfx] [PATCH 3/7] drm/i915/selftests: Relax timeout for error-interrupt reset processing Chris Wilson
2020-02-11 15:23 ` Mika Kuoppala
2020-02-11 15:33 ` Chris Wilson
2020-02-11 15:54 ` Mika Kuoppala
2020-02-11 16:00 ` Chris Wilson
2020-02-10 20:57 ` [Intel-gfx] [PATCH 4/7] drm/i915/gem: Don't leak non-persistent requests on changing engines Chris Wilson
2020-02-11 13:41 ` Tvrtko Ursulin
2020-02-11 14:15 ` Chris Wilson [this message]
2020-02-10 20:57 ` [Intel-gfx] [PATCH 5/7] drm/i915: Disable use of hwsp_cacheline for kernel_context Chris Wilson
2020-02-11 17:36 ` Mika Kuoppala
2020-02-10 20:57 ` [Intel-gfx] [PATCH 6/7] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore Chris Wilson
2020-02-10 20:57 ` [Intel-gfx] [PATCH 7/7] drm/i915/execlists: Remove preempt-to-busy roundtrip delay Chris Wilson
2020-02-12 1:08 ` Daniele Ceraolo Spurio
2020-02-14 10:10 ` Chris Wilson
2020-02-10 22:48 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915/gt: Avoid resetting ring->head outside of its timeline mutex Patchwork
2020-02-10 23:14 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-02-11 11:49 ` [Intel-gfx] [PATCH 1/7] " Andi Shyti
2020-02-11 11:58 ` Mika Kuoppala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=158143055793.2303.1572726675050076513@skylake-alporthouse-com \
--to=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
--cc=tvrtko.ursulin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).