Re: [CI] drm/xe: Disable scheduling early on FD close to avoid CAT error cascade

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A data race on the shared static message buffer causes a Power Management (PM) runtime reference leak when an exec queue is killed concurrently.
- [Medium] Double-disable of scheduling when KILL message races with TDR, potentially causing an unnecessary GT reset.
--

commit cb0a16de6bfaa13bc0da2d9c7548fe3a5d58a847
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Disable scheduling early on FD close to avoid CAT error cascade

This commit introduces a mechanism to eagerly disable scheduling on FD close
by sending a self-message (KILL) outside of the TDR path. This prevents
contexts with invalid page tables from continuing to schedule, avoiding CAT
error cascades.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff056..33ef884cd3162 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -2130,11 +2144,39 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  	return err;
>  }
>  
> +static bool guc_exec_queue_try_add_msg(struct xe_exec_queue *q,
> +				       struct xe_sched_msg *msg,
> +				       u32 opcode);
> +
> +#define STATIC_MSG_CLEANUP	0
> +#define STATIC_MSG_SUSPEND	1
> +#define STATIC_MSG_RESUME	2
> +#define STATIC_MSG_CGP_SYNC	3
> +#define STATIC_MSG_KILL		4
>  static void guc_exec_queue_kill(struct xe_exec_queue *q)
>  {
> +	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_KILL;
> +
>  	trace_xe_exec_queue_kill(q);
>  	set_exec_queue_killed(q);
>  	__suspend_fence_signal(q);
> +
> +	/*
> +	 * We eagerly send a message to ourselves to disable scheduling, as the
> +	 * TDR is serialized (i.e., only one exec queue is processed at a time).
> +	 * If an FD is closed with many exec queues, the TDR can be slower than
> +	 * the GuC scheduling contexts with invalid page tables, creating a
> +	 * cascade of CAT errors and engine resets, which is quite slow. Avoid
> +	 * this by immediately disabling scheduling outside of the TDR.
> +	 */
> +	if (kref_read(&q->refcount) && !exec_queue_wedged(q)) {
> +		struct xe_gpu_scheduler *sched = &q->guc->sched;
> +
> +		xe_sched_msg_lock(sched);
> +		guc_exec_queue_try_add_msg(q, msg, KILL);

[Severity: High]
Could this cause a Power Management (PM) runtime reference leak if multiple
threads call this concurrently?

When an exec queue is killed, this code enqueues the shared static KILL
message. Since guc_exec_queue_try_add_msg() checks list_empty(), what
happens if the background worker has already dequeued this message and is
currently processing it?

If a second concurrent kill re-enqueues the same static message,
guc_exec_queue_add_msg() will overwrite msg->opcode, temporarily clearing
the MSG_PM_REF flag:

xe_guc_submit.c:guc_exec_queue_add_msg() {
    ...
    msg->opcode = opcode & OPCODE_MASK;
    if (bound) {
        xe_pm_runtime_get_noresume(xe);
        msg->opcode |= MSG_PM_REF;
    }
    ...
}

Meanwhile, the worker might be concurrently reading msg->opcode without a
lock in guc_exec_queue_process_msg():

    bool pm_ref = !!(msg->opcode & MSG_PM_REF);

If the worker observes MSG_PM_REF as 0, could it skip xe_pm_runtime_put()
and permanently leak the PM runtime reference?


[Severity: Medium]
Could queuing this KILL message race with the TDR worker and cause an
unnecessary full GT reset?

The commit intentionally queues both the KILL message and the TDR worker
simultaneously when a queue is killed. If the KILL message executes first,
it calls disable_scheduling() which sets pending_disable.

When TDR executes guc_exec_queue_timedout_job(), it enters its disable block
because pending_disable is true, waits for it to clear, and then
unconditionally calls disable_scheduling() again:

xe_guc_submit.c:guc_exec_queue_timedout_job() {
    ...
    disable_scheduling(primary, skip_timeout_check);
    ...
}

If the GuC firmware rejects this redundant state change and does not return
a successful SCHED_DONE response, could TDR's subsequent wait time out and
trigger a stall followed by a full GT reset?

> +		xe_sched_msg_unlock(sched);
> +	}
> +
>  	xe_guc_exec_queue_trigger_cleanup(q);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825203506.1791216-1-matthew.brost@intel.com?part=1