Re: [PATCH] drm/xe: Add process name and PID to job timedout message

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2024-07-10 at 23:41 +0000, Matthew Brost wrote:
> On Wed, Jul 10, 2024 at 02:31:49PM -0700, José Roberto de Souza wrote:
> > This will be very helpful for Mesa CI, where it uses PID to match
> > the exacly test that cause timedout/GPU hang and mark that test as
> > failing.
> > 
> > Also printing the process name as it might be relavant for human
> > readers.
> > 
> 
> Always for adding useful debug info...
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_submit.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 6392381e8e697..8604055271156 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1060,7 +1060,10 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> >  	struct xe_exec_queue *q = job->q;
> >  	struct xe_gpu_scheduler *sched = &q->guc->sched;
> >  	struct xe_guc *guc = exec_queue_to_guc(q);
> > +	const char *process_name = "no process";
> > +	struct task_struct *task = NULL;
> >  	int err = -ETIME;
> > +	pid_t pid = -1;
> >  	int i = 0;
> >  	bool wedged, skip_timeout_check;
> >  
> > @@ -1157,9 +1160,19 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> >  		goto sched_enable;
> >  	}
> >  
> > -	xe_gt_notice(guc_to_gt(guc), "Timedout job: seqno=%u, lrc_seqno=%u, guc_id=%d, flags=0x%lx",
> > +	if (q->vm && q->vm->xef) {
> > +		task = get_pid_task(q->vm->xef->drm->pid, PIDTYPE_PID);
> 
> We do something simliar in devcoredump_snapshot.
> 
> Would it be worth while to have a helper like this?
> 
> struct task_struct *task xe_exec_queue_get_pid_task(struct xe_exec_queue *q)
> {
> 	if (q->vm && q->vm->xef)
> 		return get_pid_task(q->vm->xef->drm->pid, PIDTYPE_PID);;
> 
> 	return NULL;
> }

I don't think it is worthy add a function for just 2 lines of code.

> 
> Matt
> 
> > +		if (task) {
> > +			process_name = task->comm;
> > +			pid = task->pid;
> > +		}
> > +	}
> > +	xe_gt_notice(guc_to_gt(guc), "Timedout job: seqno=%u, lrc_seqno=%u, guc_id=%d, flags=0x%lx in %s [%d]",
> >  		     xe_sched_job_seqno(job), xe_sched_job_lrc_seqno(job),
> > -		     q->guc->id, q->flags);
> > +		     q->guc->id, q->flags, process_name, pid);
> > +	if (task)
> > +		put_task_struct(task);
> > +
> >  	trace_xe_sched_job_timedout(job);
> >  
> >  	if (!exec_queue_killed(q))
> > -- 
> > 2.45.2
> >