[Intel-gfx] [CI 06/20] drm/i915: Slaughter the thundering i915_wait_request herd

[Chris Wilson]

On Fri, May 20, 2016 at 01:04:13PM +0100, Tvrtko Ursulin wrote:
> >+	p = &b->waiters.rb_node;
> >+	while (*p) {
> >+		parent = *p;
> >+		if (wait->seqno == to_wait(parent)->seqno) {
> >+			/* We have multiple waiters on the same seqno, select
> >+			 * the highest priority task (that with the smallest
> >+			 * task->prio) to serve as the bottom-half for this
> >+			 * group.
> >+			 */
> >+			if (wait->task->prio > to_wait(parent)->task->prio) {
> >+				p = &parent->rb_right;
> >+				first = false;
> >+			} else
> >+				p = &parent->rb_left;
> >+		} else if (i915_seqno_passed(wait->seqno,
> >+					     to_wait(parent)->seqno)) {
> >+			p = &parent->rb_right;
> >+			if (i915_seqno_passed(seqno, to_wait(parent)->seqno))
> >+				completed = parent;
> 
> Hm don't you need the completed handling in the equal seqno case as well?

i915_seqno_passed() returnts true if seqnoA == seqnoB

> >+void intel_engine_remove_wait(struct intel_engine_cs *engine,
> >+			      struct intel_wait *wait)
> >+{
> >+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >+
> >+	/* Quick check to see if this waiter was already decoupled from
> >+	 * the tree by the bottom-half to avoid contention on the spinlock
> >+	 * by the herd.
> >+	 */
> >+	if (RB_EMPTY_NODE(&wait->node))
> >+		return;
> >+
> >+	spin_lock(&b->lock);
> >+
> >+	if (b->first_waiter == wait->task) {
> >+		struct rb_node *next;
> >+		struct task_struct *task;
> >+		const int priority = wait->task->prio;
> >+
> >+		/* We are the current bottom-half. Find the next candidate,
> >+		 * the first waiter in the queue on the remaining oldest
> >+		 * request. As multiple seqnos may complete in the time it
> >+		 * takes us to wake up and find the next waiter, we have to
> >+		 * wake up that waiter for it to perform its own coherent
> >+		 * completion check.
> >+		 */
> >+		next = rb_next(&wait->node);
> >+		if (chain_wakeup(next, priority)) {
> 
> Don't get this, next waiter my be a different seqno so how is the
> priority check relevant?

We only want to call try_to_wake_up() on processes at least as important
as us.

> Also, how can the next node be smaller priority anyway, if equal
> seqno if has be equal or greater I thought?

Next seqno can have a smaller priority (and be complete). chain_wakeup()
just asks if the next waiter is as important as ourselves (the next
waiter may be for a different seqno).
 
> Then since add_waiter will wake up one extra waiter to handle the
> missed irq case, here you may skip checking them based simply on
> task priority which seems wrong.

Correct. They will be handled by the next waiter in the qeueue, not us.
Our job is to wake as many completed (+ the potentially completed bh) as
possible without incurring undue delay for ourselves. All completed waiters
will be woken in turn as the next bh to run will look at the list and
wake up those at the the same priority as itself (+ the next potentially
completed bh).

> >+static inline bool intel_engine_wakeup(struct intel_engine_cs *engine)
> >+{
> >+	bool wakeup = false;
> >+	struct task_struct *task = READ_ONCE(engine->breadcrumbs.first_waiter);
> >+	/* Note that for this not to dangerously chase a dangling pointer,
> >+	 * the caller is responsible for ensure that the task remain valid for
> >+	 * wake_up_process() i.e. that the RCU grace period cannot expire.
> >+	 */
> 
> This gets called from hard irq context and I did not manage to
> figure out what makes it safe in the remove waiter path? Why the
> hard irq couldn't not sample NULL here?

Because we only need RCU access to task, which is provided by the hard
irq context.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre