On 20/05/16 13:19, Chris Wilson wrote:
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
I meant in the first branch, multiple waiter on the same seqno?
+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.
But what is the comment saying about having to wake up the following
waiter, because of multiple seqnos potentially completing? It says that
and then it may not wake up anyone depending on relative priorities.
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).
Who will wake up the next waiter? add_waiter will wake up one, and then
the next waiter here (in remove_waiter) may not wake up any further
based on priority.
Is the assumption that it is only possible to miss one interrupt?
+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.
What does the comment mean by saying callers are responsible for the RCU
period? From which point to which? I still can't tie whatever callers
might be doing with the unrelated hardirq.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
