[PATCH 2/3] drm/i915/active: Fix use of barriers as fence trackers
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Thu Feb 16 22:02:18 UTC 2023
Users reported oopses on list corruptions when using i915 perf with a
number of concurrently running graphics applications. Root cause analysis
pointed out to an issue in barrier processing code -- a race among perf
open / close replacing active barriers with perf requests on kernel
contexts and concurrent barrier preallocate / acquire operations performed
during user context first pin / last unpin.
When adding a request to a composite tracker, we try to use an existing
fence tracker already registered with that composite tracker. The tracker
we obtain can already track another fence, can be an idle barrier, or an
active barrier. The first case seems to be handled correctly, just passed
to __i915_active_fence_set(), but the other two cases occur problematic.
When we acquire an idle barrier, we don't claim it in any way until
__i915_active_fence_set() we call substitutes its NULL fence pointer with
that of our request's fence. But another thread looking for an idle
barrier can race with us. If that thread is collecting barriers for
preallocation, it may update the NULL fence pointer with ERR_PTR(-EAGAIN)
barrier mark, either before or after we manage to replace it with our
request fence. It can also corrupt our callback list pointers when
reusing them as an engine pointer and a preallocated barriers llist node
link, or we can corrupt their data.
If the tracker occurs a non-idle barrier in turn, we try to delete that
barrier from a list of barrier tasks it belongs to. However, while doing
that we don't respect return value of a function that performs the barrier
deletion.
A failed barrier deletion can be a side effect of the way we have that
implemented -- another thread can have temporarily emptied the list before
we manage to do that. If we ignore such failure, that other thread can
then add our fence tracker back to the barrier tasks list. Since the same
structure field of the tracker is used as a list node with both barrier
tasks lists and fence callback lists, a list corruption likely occurs.
Moreover, as soon as we delete the barrier from its list, we convert it to
an idle barrier by replacing its barrier mark with NULL and decermenting
active count of its hosting composite tracker. But as soon as we do this,
we expose that barrier to the above described idle barrier race.
Respect results of barrier deletion attempt -- use non-idle barrier only
after the deletion from its list succeeds, otherwise go back and try to
acquire another tracker. On successful deletion, don't overwrite the
barrier mark with NULL -- that's not needed since the barrier, once
deleted from its list, can no longer be acquired by any other thread, all
of them respecting deletion results. Also, don't decrease acitve counter
of the hosting composite tracker but skip follow up increase of it
instead.
Claim acquired idle barrier by marking it with ERR_PTR(-EAGAIN) barrier
mark. Serialize that operations with other threads trying to claim a
barrier and go back for picking up tracker if the current one occurs
already claimed.
Delegate all updates of fence pointers, also those populated with barrier
marks, to __i915_active_fence_set(). For that to work correctly, teach
that function to recognize and process non-idle barriers when requested.
v2: keep successfully deleted non-idle barriers still claimed with barrier
marks, delegate all ence pointer manipulations to
__i915_active_fence_set() for serialization,
- teach __i915_active_fence_set() to recognize and safely process
barriers,
- skip incrementing active count for non-idle barriers instead of
decrementing it first,
- synchronously claim idle barriers with ERR_PTR(-EAGAIN) barrier marks
as soon as acquired.
Fixes: d8af05ff38ae ("drm/i915: Allow sharing the idle-barrier from other kernel requests")
References: https://gitlab.freedesktop.org/drm/intel/-/issues/6333
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
Cc: stable at vger.kernel.org # v5.4
---
drivers/gpu/drm/i915/i915_active.c | 86 +++++++++++++++++++-----------
1 file changed, 56 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 7e2fee428ff35..6a16fcadf1bb9 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -422,14 +422,20 @@ replace_barrier(struct i915_active *ref, struct i915_active_fence *active)
* we can use it to substitute for the pending idle-barrer
* request that we want to emit on the kernel_context.
*/
- __active_del_barrier(ref, node_from_active(active));
- return true;
+ return __active_del_barrier(ref, node_from_active(active));
}
+static inline bool is_idle_barrier(struct active_node *node, u64 idx);
+static struct dma_fence *
+____i915_active_fence_set(struct i915_active_fence *active,
+ struct dma_fence *fence, bool barrier);
+
int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
{
+ u64 idx = i915_request_timeline(rq)->fence_context;
struct dma_fence *fence = &rq->fence;
struct i915_active_fence *active;
+ bool replaced;
int err;
/* Prevent reaping in case we malloc/wait while building the tree */
@@ -437,17 +443,24 @@ int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
if (err)
return err;
- active = active_instance(ref, i915_request_timeline(rq)->fence_context);
- if (!active) {
- err = -ENOMEM;
- goto out;
- }
+ do {
+ active = active_instance(ref, idx);
+ if (!active) {
+ err = -ENOMEM;
+ goto out;
+ }
- if (replace_barrier(ref, active)) {
- RCU_INIT_POINTER(active->fence, NULL);
- atomic_dec(&ref->count);
- }
- if (!__i915_active_fence_set(active, fence))
+ replaced = replace_barrier(ref, active);
+ if (replaced)
+ break;
+
+ if (!cmpxchg(__active_fence_slot(active), NULL,
+ ERR_PTR(-EAGAIN)))
+ break;
+
+ } while (is_barrier(active));
+
+ if (!____i915_active_fence_set(active, fence, true) && !replaced)
__i915_active_acquire(ref);
out:
@@ -1018,28 +1031,13 @@ void i915_request_add_active_barriers(struct i915_request *rq)
spin_unlock_irqrestore(&rq->lock, flags);
}
-/*
- * __i915_active_fence_set: Update the last active fence along its timeline
- * @active: the active tracker
- * @fence: the new fence (under construction)
- *
- * Records the new @fence as the last active fence along its timeline in
- * this active tracker, moving the tracking callbacks from the previous
- * fence onto this one. Returns the previous fence (if not already completed),
- * which the caller must ensure is executed before the new fence. To ensure
- * that the order of fences within the timeline of the i915_active_fence is
- * understood, it should be locked by the caller.
- */
-struct dma_fence *
-__i915_active_fence_set(struct i915_active_fence *active,
- struct dma_fence *fence)
+static struct dma_fence *
+____i915_active_fence_set(struct i915_active_fence *active,
+ struct dma_fence *fence, bool barrier)
{
struct dma_fence *prev;
unsigned long flags;
- if (fence == rcu_access_pointer(active->fence))
- return fence;
-
GEM_BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
/*
@@ -1064,6 +1062,12 @@ __i915_active_fence_set(struct i915_active_fence *active,
*/
spin_lock_irqsave(fence->lock, flags);
prev = xchg(__active_fence_slot(active), fence);
+ if (barrier) {
+ GEM_BUG_ON(!prev);
+ if (IS_ERR(prev))
+ prev = NULL;
+ }
+ GEM_BUG_ON(IS_ERR(prev));
if (prev) {
GEM_BUG_ON(prev == fence);
spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING);
@@ -1076,6 +1080,28 @@ __i915_active_fence_set(struct i915_active_fence *active,
return prev;
}
+/*
+ * __i915_active_fence_set: Update the last active fence along its timeline
+ * @active: the active tracker
+ * @fence: the new fence (under construction)
+ *
+ * Records the new @fence as the last active fence along its timeline in
+ * this active tracker, moving the tracking callbacks from the previous
+ * fence onto this one. Returns the previous fence (if not already completed),
+ * which the caller must ensure is executed before the new fence. To ensure
+ * that the order of fences within the timeline of the i915_active_fence is
+ * understood, it should be locked by the caller.
+ */
+struct dma_fence *
+__i915_active_fence_set(struct i915_active_fence *active,
+ struct dma_fence *fence)
+{
+ if (fence == rcu_access_pointer(active->fence))
+ return fence;
+
+ return ____i915_active_fence_set(active, fence, false);
+}
+
int i915_active_fence_set(struct i915_active_fence *active,
struct i915_request *rq)
{
--
2.25.1
More information about the Intel-gfx-trybot
mailing list