[Intel-gfx] [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[Chris Wilson]

On Fri, Apr 07, 2017 at 09:23:26AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/04/2017 18:40, Tvrtko Ursulin wrote:
> >On 06/04/2017 10:30, Chris Wilson wrote:
> >>If we attempt to wake up a waiter, who is currently checking the seqno
> >>it will be in the TASK_INTERRUPTIBLE state and ttwu will report success.
> >>However, it is actually awake and functioning -- so delay reporting the
> >>actual wake up until it sleeps.
> >>
> >>v2: Defend against !CONFIG_SMP
> >>v3: Don't filter out calls to wake_up_process
> >>
> >>References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
> >>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>---
> >> drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
> >> drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
> >> 2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>index 9ccbf26124c6..808d3a3cda0a 100644
> >>--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >>@@ -27,6 +27,12 @@
> >>
> >> #include "i915_drv.h"
> >>
> >>+#ifdef CONFIG_SMP
> >>+#define task_asleep(tsk) (!(tsk)->on_cpu)
> >>+#else
> >>+#define task_asleep(tsk) ((tsk) != current)
> >>+#endif
> >
> >It really bothers to fish into this low level info which probably isn't
> >intended to be used from the outside.
> >
> >>+
> >> static unsigned int __intel_breadcrumbs_wakeup(struct
> >>intel_breadcrumbs *b)
> >> {
> >>     struct intel_wait *wait;
> >>@@ -37,8 +43,16 @@ static unsigned int
> >>__intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
> >>     wait = b->irq_wait;
> >>     if (wait) {
> >>         result = ENGINE_WAKEUP_WAITER;
> >>-        if (wake_up_process(wait->tsk))
> >>+
> >>+        /* Be careful not to report a successful wakeup if the waiter
> >>+         * is currently processing the seqno, where it will have
> >>+         * already called set_task_state(TASK_INTERRUPTIBLE).
> >>+         */
> >>+        if (task_asleep(wait->tsk))
> >>             result |= ENGINE_WAKEUP_ASLEEP;
> >
> >And this still has the problem of not being atomic between reporting the
> >two flags. So the reported status can be false which also bothers me.
> >
> >I will need to take some more time thinking about this.
> 
> Warning, the idea below is potentially unrefined! :)
> 
> How about a scheme where on wake_up we would atomic_inc our own
> wakeup counter, and then the signaller keeps atomic_dec_and_test one
> item at a time until it has consumed all the wakeups? The code in
> intel_breadcrumbs_hangcheck only declares a missed breadcrumb if the
> wakeup counter is one, meaning this was the first wakeup?

Yeah, I'd been trying to convince myself to be happy with something like
that as well. I've been contemplating an "on-cpu" bit inside the struct
intel_wait. You will still have the complaint that the two tests are not
atomic, and I'll be handwaving that the worst-case outcome is one extra
timer period before the missed breadcrumb is spotted.

Just can't convince myself that it is worth it. Whilst I can see that if
we starve the system it is quite possible that the bottom-half never
gets run and that it may be preempted before we perform our checks, it
just seems so unlikely.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre