Re: [PATCH] drm/xe: Take job list lock in xe_sched_first_pending_job

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 11/5/2024 6:50 PM, Matthew Brost wrote:
> On Tue, Nov 05, 2024 at 05:03:27PM +0100, Nirmoy Das wrote:
>> Access to the pending_list should always happens under job_list_lock.
>>
>> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Is this showing up in any bug reports? The only user of this function is
> guc_exec_queue_stop which as stopped the scheduler worker so I think
> accessing to the pending_list is in fact safe without this lock. This is
> however a micro-optimization in a non-hot path which I think is frowned
> upon.

This showed up in coverity report so no real bug report. Also agree that with current usage this won't

cause any issue so avoided adding Cc to stable.

>
> So I think with above the fixes tag is not strickly required.

I will remove the Fixes tag before merging.

>
> For the patch though:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>


Thanks,

Nirmoy

>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gpu_scheduler.h | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> index 64b2ae6839db..c250ea773491 100644
>> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> @@ -71,8 +71,14 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
>>  static inline
>>  struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
>>  {
>> -	return list_first_entry_or_null(&sched->base.pending_list,
>> -					struct xe_sched_job, drm.list);
>> +	struct xe_sched_job *job;
>> +
>> +	spin_lock(&sched->base.job_list_lock);
>> +	job = list_first_entry_or_null(&sched->base.pending_list,
>> +				       struct xe_sched_job, drm.list);
>> +	spin_unlock(&sched->base.job_list_lock);
>> +
>> +	return job;
>>  }
>>  
>>  static inline int
>> -- 
>> 2.46.0
>>