Skip to content

Commit

Permalink
drm/i915: Slaughter the thundering i915_wait_request herd
Browse files Browse the repository at this point in the history
One particularly stressful scenario consists of many independent tasks
all competing for GPU time and waiting upon the results (e.g. realtime
transcoding of many, many streams). One bottleneck in particular is that
each client waits on its own results, but every client is woken up after
every batchbuffer - hence the thunder of hooves as then every client must
do its heavyweight dance to read a coherent seqno to see if it is the
lucky one.

Ideally, we only want one client to wake up after the interrupt and
check its request for completion. Since the requests must retire in
order, we can select the first client on the oldest request to be woken.
Once that client has completed his wait, we can then wake up the
next client and so on. However, all clients then incur latency as every
process in the chain may be delayed for scheduling - this may also then
cause some priority inversion. To reduce the latency, when a client
is added or removed from the list, we scan the tree for completed
seqno and wake up all the completed waiters in parallel.

Using igt/benchmarks/gem_latency, we can demonstrate this effect. The
benchmark measures the number of GPU cycles between completion of a
batch and the client waking up from a call to wait-ioctl. With many
concurrent waiters, with each on a different request, we observe that
the wakeup latency before the patch scales nearly linearly with the
number of waiters (before external factors kick in making the scaling much
worse). After applying the patch, we can see that only the single waiter
for the request is being woken up, providing a constant wakeup latency
for every operation. However, the situation is not quite as rosy for
many waiters on the same request, though to the best of my knowledge this
is much less likely in practice. Here, we can observe that the
concurrent waiters incur extra latency from being woken up by the
solitary bottom-half, rather than directly by the interrupt. This
appears to be scheduler induced (having discounted adverse effects from
having a rbtree walk/erase in the wakeup path), each additional
wake_up_process() costs approximately 1us on big core. Another effect of
performing the secondary wakeups from the first bottom-half is the
incurred delay this imposes on high priority threads - rather than
immediately returning to userspace and leaving the interrupt handler to
wake the others.

To offset the delay incurred with additional waiters on a request, we
could use a hybrid scheme that did a quick read in the interrupt handler
and dequeued all the completed waiters (incurring the overhead in the
interrupt handler, not the best plan either as we then incur GPU
submission latency) but we would still have to wake up the bottom-half
every time to do the heavyweight slow read. Or we could only kick the
waiters on the seqno with the same priority as the current task (i.e. in
the realtime waiter scenario, only it is woken up immediately by the
interrupt and simply queues the next waiter before returning to userspace,
minimising its delay at the expense of the chain, and also reducing
contention on its scheduler runqueue). This is effective at avoid long
pauses in the interrupt handler and at avoiding the extra latency in
realtime/high-priority waiters.

v2: Convert from a kworker per engine into a dedicated kthread for the
bottom-half.
v3: Rename request members and tweak comments.
v4: Use a per-engine spinlock in the breadcrumbs bottom-half.
v5: Fix race in locklessly checking waiter status and kicking the task on
adding a new waiter.
v6: Fix deciding when to force the timer to hide missing interrupts.
v7: Move the bottom-half from the kthread to the first client process.
v8: Reword a few comments
v9: Break the busy loop when the interrupt is unmasked or has fired.
v10: Comments, unnecessary churn, better debugging from Tvrtko
v11: Wake all completed waiters on removing the current bottom-half to
reduce the latency of waking up a herd of clients all waiting on the
same request.
v12: Rearrange missed-interrupt fault injection so that it works with
igt/drv_missed_irq_hang
v13: Rename intel_breadcrumb and friends to intel_wait in preparation
for signal handling.
v14: RCU commentary, assert_spin_locked
v15: Hide BUG_ON behind the compiler; report on gem_latency findings.
v16: Sort seqno-groups by priority so that first-waiter has the highest
task priority (and so avoid priority inversion).
v17: Add waiters to post-mortem GPU hang state.
v18: Return early for a completed wait after acquiring the spinlock.
Avoids adding ourselves to the tree if the is already complete, and
skips the awkward question of why we don't do completion wakeups for
waits earlier than or equal to ourselves.
v19: Prepare for init_breadcrumbs to fail. Later patches may want to
allocate during init, so be prepared to propagate back the error code.

Testcase: igt/gem_concurrent_blit
Testcase: igt/benchmarks/gem_latency
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: "Goel, Akash" <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> #v18
Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-6-git-send-email-chris@chris-wilson.co.uk
  • Loading branch information
Chris Wilson committed Jul 1, 2016
1 parent 1f15b76 commit 688e6c7
Show file tree
Hide file tree
Showing 10 changed files with 648 additions and 110 deletions.
1 change: 1 addition & 0 deletions drivers/gpu/drm/i915/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ i915-y += i915_cmd_parser.o \
i915_gem_userptr.o \
i915_gpu_error.o \
i915_trace_points.o \
intel_breadcrumbs.o \
intel_lrc.o \
intel_mocs.o \
intel_ringbuffer.o \
Expand Down
16 changes: 15 additions & 1 deletion drivers/gpu/drm/i915/i915_debugfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -788,10 +788,22 @@ static int i915_gem_request_info(struct seq_file *m, void *data)
static void i915_ring_seqno_info(struct seq_file *m,
struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
struct rb_node *rb;

seq_printf(m, "Current sequence (%s): %x\n",
engine->name, engine->get_seqno(engine));
seq_printf(m, "Current user interrupts (%s): %x\n",
engine->name, READ_ONCE(engine->user_interrupts));

spin_lock(&b->lock);
for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
struct intel_wait *w = container_of(rb, typeof(*w), node);

seq_printf(m, "Waiting (%s): %s [%d] on %x\n",
engine->name, w->tsk->comm, w->tsk->pid, w->seqno);
}
spin_unlock(&b->lock);
}

static int i915_gem_seqno_info(struct seq_file *m, void *data)
Expand Down Expand Up @@ -1428,6 +1440,8 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
engine->hangcheck.seqno,
seqno[id],
engine->last_submitted_seqno);
seq_printf(m, "\twaiters? %d\n",
intel_engine_has_waiter(engine));
seq_printf(m, "\tuser interrupts = %x [current %x]\n",
engine->hangcheck.user_interrupts,
READ_ONCE(engine->user_interrupts));
Expand Down Expand Up @@ -2415,7 +2429,7 @@ static int count_irq_waiters(struct drm_i915_private *i915)
int count = 0;

for_each_engine(engine, i915)
count += engine->irq_refcount;
count += intel_engine_has_waiter(engine);

return count;
}
Expand Down
40 changes: 38 additions & 2 deletions drivers/gpu/drm/i915/i915_drv.h
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ struct drm_i915_error_state {
bool valid;
/* Software tracked state */
bool waiting;
int num_waiters;
int hangcheck_score;
enum intel_ring_hangcheck_action hangcheck_action;
int num_requests;
Expand Down Expand Up @@ -551,6 +552,12 @@ struct drm_i915_error_state {
u32 tail;
} *requests;

struct drm_i915_error_waiter {
char comm[TASK_COMM_LEN];
pid_t pid;
u32 seqno;
} *waiters;

struct {
u32 gfx_mode;
union {
Expand Down Expand Up @@ -1429,7 +1436,7 @@ struct i915_gpu_error {
#define I915_STOP_RING_ALLOW_WARN (1 << 30)

/* For missed irq/seqno simulation. */
unsigned int test_irq_rings;
unsigned long test_irq_rings;
};

enum modeset_restore {
Expand Down Expand Up @@ -3064,7 +3071,6 @@ ibx_disable_display_interrupt(struct drm_i915_private *dev_priv, uint32_t bits)
ibx_display_interrupt_update(dev_priv, bits, 0);
}


/* i915_gem.c */
int i915_gem_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
Expand Down Expand Up @@ -3975,4 +3981,34 @@ static inline void i915_trace_irq_get(struct intel_engine_cs *engine,
i915_gem_request_assign(&engine->trace_irq_req, req);
}

static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
{
/* Ensure our read of the seqno is coherent so that we
* do not "miss an interrupt" (i.e. if this is the last
* request and the seqno write from the GPU is not visible
* by the time the interrupt fires, we will see that the
* request is incomplete and go back to sleep awaiting
* another interrupt that will never come.)
*
* Strictly, we only need to do this once after an interrupt,
* but it is easier and safer to do it every time the waiter
* is woken.
*/
if (i915_gem_request_completed(req, false))
return true;

/* We need to check whether any gpu reset happened in between
* the request being submitted and now. If a reset has occurred,
* the seqno will have been advance past ours and our request
* is complete. If we are in the process of handling a reset,
* the request is effectively complete as the rendering will
* be discarded, but we need to return in order to drop the
* struct_mutex.
*/
if (i915_reset_in_progress(&req->i915->gpu_error))
return true;

return false;
}

#endif
143 changes: 53 additions & 90 deletions drivers/gpu/drm/i915/i915_gem.c
Original file line number Diff line number Diff line change
Expand Up @@ -1343,17 +1343,6 @@ i915_gem_check_wedge(unsigned reset_counter, bool interruptible)
return 0;
}

static void fake_irq(unsigned long data)
{
wake_up_process((struct task_struct *)data);
}

static bool missed_irq(struct drm_i915_private *dev_priv,
struct intel_engine_cs *engine)
{
return test_bit(engine->id, &dev_priv->gpu_error.missed_irq_rings);
}

static unsigned long local_clock_us(unsigned *cpu)
{
unsigned long t;
Expand Down Expand Up @@ -1386,7 +1375,7 @@ static bool busywait_stop(unsigned long timeout, unsigned cpu)
return this_cpu != cpu;
}

static int __i915_spin_request(struct drm_i915_gem_request *req, int state)
static bool __i915_spin_request(struct drm_i915_gem_request *req, int state)
{
unsigned long timeout;
unsigned cpu;
Expand All @@ -1401,17 +1390,14 @@ static int __i915_spin_request(struct drm_i915_gem_request *req, int state)
* takes to sleep on a request, on the order of a microsecond.
*/

if (req->engine->irq_refcount)
return -EBUSY;

/* Only spin if we know the GPU is processing this request */
if (!i915_gem_request_started(req, true))
return -EAGAIN;
return false;

timeout = local_clock_us(&cpu) + 5;
while (!need_resched()) {
do {
if (i915_gem_request_completed(req, true))
return 0;
return true;

if (signal_pending_state(state, current))
break;
Expand All @@ -1420,12 +1406,9 @@ static int __i915_spin_request(struct drm_i915_gem_request *req, int state)
break;

cpu_relax_lowlatency();
}

if (i915_gem_request_completed(req, false))
return 0;
} while (!need_resched());

return -EAGAIN;
return false;
}

/**
Expand All @@ -1450,123 +1433,97 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
s64 *timeout,
struct intel_rps_client *rps)
{
struct intel_engine_cs *engine = i915_gem_request_get_engine(req);
struct drm_i915_private *dev_priv = req->i915;
const bool irq_test_in_progress =
ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_engine_flag(engine);
int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
DEFINE_WAIT(reset);
DEFINE_WAIT(wait);
unsigned long timeout_expire;
struct intel_wait wait;
unsigned long timeout_remain;
s64 before = 0; /* Only to silence a compiler warning. */
int ret;
int ret = 0;

WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
might_sleep();

if (list_empty(&req->list))
return 0;

if (i915_gem_request_completed(req, true))
return 0;

timeout_expire = 0;
timeout_remain = MAX_SCHEDULE_TIMEOUT;
if (timeout) {
if (WARN_ON(*timeout < 0))
return -EINVAL;

if (*timeout == 0)
return -ETIME;

timeout_expire = jiffies + nsecs_to_jiffies_timeout(*timeout);
timeout_remain = nsecs_to_jiffies_timeout(*timeout);

/*
* Record current time in case interrupted by signal, or wedged.
*/
before = ktime_get_raw_ns();
}

if (INTEL_INFO(dev_priv)->gen >= 6)
gen6_rps_boost(dev_priv, rps, req->emitted_jiffies);

trace_i915_gem_request_wait_begin(req);

/* Optimistic spin for the next jiffie before touching IRQs */
ret = __i915_spin_request(req, state);
if (ret == 0)
goto out;
if (INTEL_INFO(req->i915)->gen >= 6)
gen6_rps_boost(req->i915, rps, req->emitted_jiffies);

if (!irq_test_in_progress && WARN_ON(!engine->irq_get(engine))) {
ret = -ENODEV;
goto out;
}
/* Optimistic spin for the next ~jiffie before touching IRQs */
if (__i915_spin_request(req, state))
goto complete;

add_wait_queue(&dev_priv->gpu_error.wait_queue, &reset);
for (;;) {
struct timer_list timer;
set_current_state(state);
add_wait_queue(&req->i915->gpu_error.wait_queue, &reset);

prepare_to_wait(&engine->irq_queue, &wait, state);

/* We need to check whether any gpu reset happened in between
* the request being submitted and now. If a reset has occurred,
* the seqno will have been advance past ours and our request
* is complete. If we are in the process of handling a reset,
* the request is effectively complete as the rendering will
* be discarded, but we need to return in order to drop the
* struct_mutex.
intel_wait_init(&wait, req->seqno);
if (intel_engine_add_wait(req->engine, &wait))
/* In order to check that we haven't missed the interrupt
* as we enabled it, we need to kick ourselves to do a
* coherent check on the seqno before we sleep.
*/
if (i915_reset_in_progress(&dev_priv->gpu_error)) {
ret = 0;
break;
}

if (i915_gem_request_completed(req, false)) {
ret = 0;
break;
}
goto wakeup;

for (;;) {
if (signal_pending_state(state, current)) {
ret = -ERESTARTSYS;
break;
}

if (timeout && time_after_eq(jiffies, timeout_expire)) {
ret = -ETIME;
break;
}

/* Ensure that even if the GPU hangs, we get woken up.
*
* However, note that if no one is waiting, we never notice
* a gpu hang. Eventually, we will have to wait for a resource
* held by the GPU and so trigger a hangcheck. In the most
* pathological case, this will be upon memory starvation!
*/
i915_queue_hangcheck(dev_priv);

timer.function = NULL;
if (timeout || missed_irq(dev_priv, engine)) {
unsigned long expire;
i915_queue_hangcheck(req->i915);

setup_timer_on_stack(&timer, fake_irq, (unsigned long)current);
expire = missed_irq(dev_priv, engine) ? jiffies + 1 : timeout_expire;
mod_timer(&timer, expire);
timeout_remain = io_schedule_timeout(timeout_remain);
if (timeout_remain == 0) {
ret = -ETIME;
break;
}

io_schedule();

if (timer.function) {
del_singleshot_timer_sync(&timer);
destroy_timer_on_stack(&timer);
}
}
remove_wait_queue(&dev_priv->gpu_error.wait_queue, &reset);
if (intel_wait_complete(&wait))
break;

if (!irq_test_in_progress)
engine->irq_put(engine);
set_current_state(state);

finish_wait(&engine->irq_queue, &wait);
wakeup:
/* Carefully check if the request is complete, giving time
* for the seqno to be visible following the interrupt.
* We also have to check in case we are kicked by the GPU
* reset in order to drop the struct_mutex.
*/
if (__i915_request_irq_complete(req))
break;
}
remove_wait_queue(&req->i915->gpu_error.wait_queue, &reset);

out:
intel_engine_remove_wait(req->engine, &wait);
__set_current_state(TASK_RUNNING);
complete:
trace_i915_gem_request_wait_end(req);

if (timeout) {
Expand Down Expand Up @@ -2796,6 +2753,12 @@ i915_gem_init_seqno(struct drm_i915_private *dev_priv, u32 seqno)
}
i915_gem_retire_requests(dev_priv);

/* If the seqno wraps around, we need to clear the breadcrumb rbtree */
if (!i915_seqno_passed(seqno, dev_priv->next_seqno)) {
while (intel_kick_waiters(dev_priv))
yield();
}

/* Finally reset hw state */
for_each_engine(engine, dev_priv)
intel_ring_init_seqno(engine, seqno);
Expand Down
Loading

0 comments on commit 688e6c7

Please sign in to comment.