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

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jul 11, 2024 at 07:35:43AM -0600, Souza, Jose wrote:
> 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.
> 

Typcially I agree but my counter here is we did have a bug for this [1]
so one point for this logic might be better.

Anyways I'm not going to block this patch on a bikeshed.

As is or with helper:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

[1] https://patchwork.freedesktop.org/patch/596388/?series=134265&rev=1

> > 
> > 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
> > > 
>