Re: [PATCH v4 2/3] 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 4 potential issue(s) to consider:
- [High] drm/xe: missing killed check in guc_exec_queue_run_job() after early disable
- [High] drm/xe: __guc_exec_queue_process_msg_kill() disables live multi-queue primary
- [High] drm/xe: missing ct.wq wakeup in handle_sched_done() for killed queues
- [High] drm/xe: guc_exec_queue_kill() called on zero-refcount queue in pause_abort
--

commit f3abc41f0625f68bc7ba67b2b80c949770c16076
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Disable scheduling early on FD close to avoid CAT error cascade

This commit adds a STATIC_MSG_KILL scheduler message sent from
guc_exec_queue_kill() to disable GuC context scheduling ahead of the
serialized TDR cleanup path when an exec queue is killed.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 924024f6d0b24..1fbc8cd767675 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2058,12 +2058,45 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg,
>  		guc_exec_queue_send_cgp_sync(q, 0);
>  }
>  
> +static void __guc_exec_queue_process_msg_kill(struct xe_sched_msg *msg,
> +					      bool bound)
> +{
> +	struct xe_exec_queue *q = msg->private_data;
> +	struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);

[Severity: High]
What happens in __guc_exec_queue_process_msg_kill() when a single secondary
queue in an active multi-queue group is killed via
xe_exec_queue_destroy_ioctl() or the error path of
xe_exec_queue_create_ioctl() while the primary queue remains active?

In xe_exec_queue_destroy_ioctl():

drivers/gpu/drm/xe/xe_exec_queue.c:xe_exec_queue_destroy_ioctl() {
	...
	xe_exec_queue_kill(q);

	trace_xe_exec_queue_close(q);
	xe_exec_queue_put(q);
	...
}

guc_exec_queue_kill() queues the KILL message before xe_exec_queue_put(q) is
called (and in preempt-fence mode, q->lr.pfence also holds a reference on q
until preempt_fence_work_func() runs). When __guc_exec_queue_process_msg_kill()
processes that secondary queue's message while kref_read(&q->refcount) is
non-zero, it resolves primary = xe_exec_queue_multi_queue_primary(q) and calls
disable_scheduling(primary, true) on the live, non-killed primary queue:

drivers/gpu/drm/xe/xe_guc_submit.c:disable_scheduling() {
	...
	if (immediate && !xe_exec_queue_is_multi_queue_secondary(q))
		set_min_preemption_timeout(guc, q);
	clear_exec_queue_enabled(q);
	set_exec_queue_pending_disable(q);
	...
}

Doesn't this overwrite the live primary context's preemption timeout to 1us,
disable GuC scheduling for the entire active multi-queue group, and leave
EXEC_QUEUE_STATE_PENDING_DISABLE set on the non-killed primary queue? If
subsequent jobs run or the primary queue is resumed while that disable is in
flight, guc_exec_queue_run_job() and enable_scheduling() will hit
xe_gt_assert() on exec_queue_pending_disable(primary), or submit_exec_queue()
will see !exec_queue_enabled(primary) and send a concurrent
GUC_CONTEXT_ENABLE while GUC_CONTEXT_DISABLE is still pending.

> +	struct xe_guc *guc = exec_queue_to_guc(primary);
> +	int ret;
> +
> +	/* At zero references, the queued CLEANUP message handles disabling. */
> +	if (!kref_read(&q->refcount) || !bound || exec_queue_wedged(q))
> +		return;
> +
> +	ret = wait_event_timeout(guc->ct.wq,
> +				 (!exec_queue_pending_enable(primary) &&
> +				  !exec_queue_pending_disable(primary)) ||
> +				 xe_guc_read_stopped(guc) ||
> +				 vf_recovery(guc), HZ * 5);
> +	if (!ret && !vf_recovery(guc)) {
> +		xe_gt_warn(primary->gt, "Pending enable/disable failed to respond on kill, guc_id=%d\n",
> +			   primary->guc->id);
> +		xe_gt_reset_async(primary->gt);
> +		xe_sched_tdr_queue_imm(&q->guc->sched);
> +		return;
> +	}
> +
> +	if (xe_guc_read_stopped(guc) || vf_recovery(guc))
> +		return;
> +
> +	if (exec_queue_enabled(primary))
> +		disable_scheduling(primary, true);
> +}

[Severity: High]
Can calling disable_scheduling(primary, true) here before TDR stops the
scheduler trip the state assertion in guc_exec_queue_run_job()?

Prior to this change, EXEC_QUEUE_STATE_PENDING_DISABLE was only set when a
queue was either suspended, banned (with scheduler submission stopped), or
destroyed at zero refcount, and guc_exec_queue_run_job() asserts this
invariant:

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_run_job() {
	...
	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
		     exec_queue_banned(q) || exec_queue_suspended(q));
	...
}

Because guc_exec_queue_kill() only sets EXEC_QUEUE_STATE_KILLED (not BANNED or
SUSPENDED) and does not stop drm_sched_run_job_work (which is only stopped
later when guc_exec_queue_timedout_job() runs on gt->ordered_wq), if another
job is queued on q->entity and guc_exec_queue_run_job() executes on submit_wq
after __guc_exec_queue_process_msg_kill() sets
EXEC_QUEUE_STATE_PENDING_DISABLE and before the GuC SCHED_DONE G2H clears it
or TDR bans the queue, won't guc_exec_queue_run_job() trigger this
xe_gt_assert()?

[Severity: High]
Also, when disable_scheduling(primary, true) is called from
__guc_exec_queue_process_msg_kill(), what wakes guc->ct.wq when the GuC
replies with XE_GUC_ACTION_SCHED_CONTEXT_MODE_DONE?

In handle_sched_done(), when runnable_state == 0 and !was_pending, the wakeup
of guc->ct.wq is gated on exec_queue_banned(q) (and placed before
clear_exec_queue_pending_disable(q)):

drivers/gpu/drm/xe/xe_guc_submit.c:handle_sched_done() {
	...
		if (!was_pending) {
			if (exec_queue_banned(q)) {
				smp_wmb();
				wake_up_all(&guc->ct.wq);
			}
			if (exec_queue_destroyed(q)) {
				clear_exec_queue_pending_disable(q);
				deregister_exec_queue(guc, q);
			} else {
				clear_exec_queue_pending_disable(q);
			}
		}
	...
}

Because a killed queue only has EXEC_QUEUE_STATE_KILLED set and
exec_queue_banned(primary) is false, handle_sched_done() clears
EXEC_QUEUE_STATE_PENDING_DISABLE without waking guc->ct.wq (and the follow-up
commit "drm/xe: Synchronously wait for queue kill on FD close" only wakes
kill_wq via guc_wake_kill_waiters(), not guc->ct.wq).

Won't any waiter sleeping on guc->ct.wq for !exec_queue_pending_disable()
stall for the 5-second timeout and trigger a spurious xe_gt_reset_async()? In
particular:

1. __guc_exec_queue_process_msg_kill() or guc_exec_queue_timedout_job() on a
   secondary queue in the same multi-queue group waiting on guc->ct.wq for
   !exec_queue_pending_disable(primary).
2. disable_scheduling_deregister() during CLEANUP when a queue is killed and
   put (e.g. via xe_exec_queue_destroy_ioctl(), xe_vm_close_and_put(), or
   xe_file_close() in this commit) while KILL's disable_scheduling() is in
   flight. If disable_scheduling_deregister() times out waiting on guc->ct.wq,
   it calls xe_gt_reset_async() and returns early before calling
   set_exec_queue_destroyed(q), which causes guc_exec_queue_stop() during reset
   to skip guc_exec_queue_destroy_async(q) and leak the exec queue and guc_id.
3. Similarly, if primary had an in-flight suspend on non-VF when KILL ran,
   __guc_exec_queue_process_msg_kill() waits on guc->ct.wq while
   suspend_fence_signal() -> suspend_wait_wake() only wakes
   q->guc->suspend_wait.

In addition, in guc_exec_queue_timedout_job():

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_timedout_job() {
	...
	if (!xe_device_is_in_reset(xe) && !wedged &&
	    (exec_queue_enabled(primary) || exec_queue_pending_disable(primary))) {
		...
		if (xe_uc_fw_is_running(&guc->fw)) {
			ret = wait_event_timeout(guc->ct.wq,
						 (!exec_queue_pending_enable(primary) &&
						  !exec_queue_pending_disable(primary)) ||
						 xe_guc_read_stopped(guc) ||
						 vf_recovery(guc), HZ * 5);
			...
			disable_scheduling(primary, skip_timeout_check);
		}
	...
}

If TDR enters this block because KILL set exec_queue_pending_disable(primary)
(while exec_queue_enabled(primary) is already cleared), won't TDR wait for
pending_disable(primary) to clear and then unconditionally call
disable_scheduling(primary, skip_timeout_check) a second time on the
already-disabled context?

[ ... ]

> @@ -2224,8 +2260,21 @@ 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;
> +
> +	xe_gt_assert(q->gt, kref_read(&q->refcount));

[Severity: High]
Can this xe_gt_assert(q->gt, kref_read(&q->refcount)) fire when
guc_exec_queue_kill() is called from xe_guc_submit_pause_abort()?

When the final reference to an exec queue is dropped via xe_exec_queue_put(),
q->refcount becomes 0 and guc_exec_queue_destroy() queues STATIC_MSG_CLEANUP,
but q remains in guc->submission_state.exec_queue_lookup until the async
destroy_async worker runs guc_exec_queue_fini() -> release_guc_id(guc, q).

In xe_guc_submit_pause_abort() (called from xe_guc_submit_wedge(),
vf_post_migration_abort(), and guc_submit_fini()):

drivers/gpu/drm/xe/xe_guc_submit.c:xe_guc_submit_pause_abort() {
	...
	mutex_lock(&guc->submission_state.lock);
	xa_for_each(&guc->submission_state.exec_queue_lookup, index, q) {
		struct xe_gpu_scheduler *sched = &q->guc->sched;

		/* Prevent redundant attempts to stop parallel queues */
		if (q->guc->id != index)
			continue;

		xe_sched_submission_start(sched);
		guc_exec_queue_kill(q);
	}
	mutex_unlock(&guc->submission_state.lock);
}

And in guc_submit_fini(), the final reference on wedged queues in
exec_queue_lookup is explicitly dropped right before calling
xe_guc_submit_pause_abort():

drivers/gpu/drm/xe/xe_guc_submit.c:guc_submit_fini() {
	...
	mutex_lock(&guc->submission_state.lock);
	xa_for_each(&guc->submission_state.exec_queue_lookup, index, q) {
		if (exec_queue_wedged(q)) {
			mutex_unlock(&guc->submission_state.lock);
			xe_exec_queue_put(q);
			mutex_lock(&guc->submission_state.lock);
		}
	}
	mutex_unlock(&guc->submission_state.lock);
	...
	xe_guc_submit_pause_abort(guc);
}

If xe_guc_submit_pause_abort() encounters a queue in exec_queue_lookup whose
refcount has already reached 0, won't it trigger the new xe_gt_assert() in
guc_exec_queue_kill()? Furthermore, if EXEC_QUEUE_STATE_WEDGED is not set on q
(such as in the reset/abort branch of xe_guc_submit_wedge() or
vf_post_migration_abort()) and guc_exec_queue_destroy_async() is already
scheduled, queuing STATIC_MSG_KILL acquires a runtime PM reference via
MSG_PM_REF that can be leaked when guc_exec_queue_fini() -> xe_sched_fini()
cancels work_process_msg without draining sched->msgs.

> +
>  	trace_xe_exec_queue_kill(q);
>  	set_exec_queue_killed(q);

[ ... ]

>  	if (!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);
>  		xe_sched_msg_unlock(sched);
>  	}
>  
>  	xe_guc_exec_queue_trigger_cleanup(q);
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016195904.1377149-1-daniel.charles@intel.com?part=2