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

[Tvrtko Ursulin]

On 05/04/2017 13:47, 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
>
> 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 | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9ccbf26124c6..4fdf7868e2f1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -27,6 +27,23 @@
>
>  #include "i915_drv.h"
>
> +#ifdef CONFIG_SMP
> +#define task_asleep(tsk) (!(tsk)->on_cpu)
> +#else
> +#define task_asleep(tsk) ((tsk) != current)
> +#endif

I've looked and on_cpu seems to be a boolean indicating whether a task 
is currently running. Which means on UP tsk != current is a correct 
replacement. However ...

> +
> +static inline bool __wake_up_sleeper(struct task_struct *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). We first check whether
> +	 * the task is currently asleep before calling ttwu, and then we
> +	 * only report success if we were the ones to then trigger the wakeup.
> +	 */
> +	return task_asleep(tsk) && wake_up_process(tsk);

... I don't see why on SMP task couldn't get "on_cpu" between the 
task_asleep() and wake_up_process? That would then foil the test and 
just shrink the race window a bit.

Regards,

Tvrtko

> +}
> +
>  static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>  {
>  	struct intel_wait *wait;
> @@ -37,7 +54,7 @@ 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))
> +		if (__wake_up_sleeper(wait->tsk))
>  			result |= ENGINE_WAKEUP_ASLEEP;
>  	}
>
> @@ -198,7 +215,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>
>  	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
>  		RB_CLEAR_NODE(&wait->node);
> -		if (wake_up_process(wait->tsk) && wait == first)
> +		if (__wake_up_sleeper(wait->tsk) && wait == first)
>  			missed_breadcrumb(engine);
>  	}
>  	b->waiters = RB_ROOT;
>