[Intel-gfx] [PATCH v3] drm/i915: Add unit tests for the breadcrumb rbtree, wakeups

[Chris Wilson]

On Wed, Feb 01, 2017 at 04:57:53PM +0000, Tvrtko Ursulin wrote:
> >+static bool wait_for_ready(struct igt_wakeup *w)
> >+{
> >+	DEFINE_WAIT(ready);
> >+
> >+	if (atomic_dec_and_test(w->done))
> >+		wake_up_atomic_t(w->done);
> >+
> >+	if (test_bit(STOP, &w->flags))
> >+		goto out;
> >+
> >+	set_bit(WAITING, &w->flags);
> >+	for (;;) {
> >+		prepare_to_wait(w->wq, &ready, TASK_INTERRUPTIBLE);
> >+		if (atomic_read(w->ready) == 0)
> >+			break;
> >+
> >+		schedule();
> >+	}
> >+	finish_wait(w->wq, &ready);
> >+	clear_bit(WAITING, &w->flags);
> >+
> >+out:
> >+	if (atomic_dec_and_test(w->set))
> >+		wake_up_atomic_t(w->set);
> >+
> >+	return !test_bit(STOP, &w->flags);
> >+}



> >+	atomic_set(&ready, count);
> >+	for (n = 0; n < count; n++) {
> >+		waiters[n].wq = &wq;
> >+		waiters[n].ready = &ready;
> >+		waiters[n].set = &set;
> >+		waiters[n].done = &done;
> >+		waiters[n].engine = engine;
> >+		waiters[n].flags = 0;
> >+
> >+		waiters[n].tsk = kthread_run(igt_wakeup_thread, &waiters[n],
> >+					     "i915/igt:%d", n);
> >+		if (IS_ERR(waiters[n].tsk))
> >+			goto out_waiters;
> >+
> >+		get_task_struct(waiters[n].tsk);
> >+	}
> >+
> >+	for (step = 1; step <= max_seqno; step <<= 1) {
> >+		u32 seqno;
> >+
> >+		/* The waiter threads start paused as we assign them a random
> >+		 * seqno and reset the engine. Once the engine is reset,
> >+		 * we signal that the threads may begin their, and we wait
> >+		 * until all threads are woken.
> >+		 */
> >+		for (n = 0; n < count; n++) {
> >+			GEM_BUG_ON(!test_bit(WAITING, &waiters[n].flags));
> 
> Looks like a race condition between thread startup and checking this
> bit. I think the assert is just unnecessary.

I liked it for the document that we only update the waiters state and
reset the engine whilst the threads are idle. You're right about the
startup race, but we can start with the flag set. Maybe s/WAITING/IDLE/
to try and avoid reusing "wait" too often.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre