Re: [Intel-xe] [PATCH v2 4/9] drm/sched: Split free_job into own work item

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

On Fri, Aug 25, 2023 at 10:02:32AM +0200, Christian König wrote:
> Am 25.08.23 um 04:58 schrieb Matthew Brost:
> > On Fri, Aug 25, 2023 at 01:04:10AM +0200, Danilo Krummrich wrote:
> > > On Thu, Aug 10, 2023 at 07:31:32PM -0700, Matthew Brost wrote:
> > > > Rather than call free_job and run_job in same work item have a dedicated
> > > > work item for each. This aligns with the design and intended use of work
> > > > queues.
> > > > 
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/scheduler/sched_main.c | 137 ++++++++++++++++++-------
> > > >   include/drm/gpu_scheduler.h            |   8 +-
> > > >   2 files changed, 106 insertions(+), 39 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index cede47afc800..b67469eac179 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> > > >    * drm_sched_rq_select_entity_rr - Select an entity which could provide a job to run
> > > >    *
> > > >    * @rq: scheduler run queue to check.
> > > > + * @dequeue: dequeue selected entity
> > > >    *
> > > >    * Try to find a ready entity, returns NULL if none found.
> > > >    */
> > > >   static struct drm_sched_entity *
> > > > -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > > > +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
> > > >   {
> > > >   	struct drm_sched_entity *entity;
> > > > @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > > >   	if (entity) {
> > > >   		list_for_each_entry_continue(entity, &rq->entities, list) {
> > > >   			if (drm_sched_entity_is_ready(entity)) {
> > > > -				rq->current_entity = entity;
> > > > -				reinit_completion(&entity->entity_idle);
> > > > +				if (dequeue) {
> > > > +					rq->current_entity = entity;
> > > > +					reinit_completion(&entity->entity_idle);
> > > > +				}
> > > >   				spin_unlock(&rq->lock);
> > > >   				return entity;
> > > >   			}
> > > > @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > > >   	list_for_each_entry(entity, &rq->entities, list) {
> > > >   		if (drm_sched_entity_is_ready(entity)) {
> > > > -			rq->current_entity = entity;
> > > > -			reinit_completion(&entity->entity_idle);
> > > > +			if (dequeue) {
> > > > +				rq->current_entity = entity;
> > > > +				reinit_completion(&entity->entity_idle);
> > > > +			}
> > > >   			spin_unlock(&rq->lock);
> > > >   			return entity;
> > > >   		}
> > > > @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > > >    * drm_sched_rq_select_entity_fifo - Select an entity which provides a job to run
> > > >    *
> > > >    * @rq: scheduler run queue to check.
> > > > + * @dequeue: dequeue selected entity
> > > >    *
> > > >    * Find oldest waiting ready entity, returns NULL if none found.
> > > >    */
> > > >   static struct drm_sched_entity *
> > > > -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > > > +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
> > > >   {
> > > >   	struct rb_node *rb;
> > > > @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > > >   		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> > > >   		if (drm_sched_entity_is_ready(entity)) {
> > > > -			rq->current_entity = entity;
> > > > -			reinit_completion(&entity->entity_idle);
> > > > +			if (dequeue) {
> > > > +				rq->current_entity = entity;
> > > > +				reinit_completion(&entity->entity_idle);
> > > > +			}
> > > >   			break;
> > > >   		}
> > > >   	}
> > > > @@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > > >   }
> > > >   /**
> > > > - * drm_sched_submit_queue - scheduler queue submission
> > > > + * drm_sched_run_job_queue - queue job submission
> > > >    * @sched: scheduler instance
> > > >    */
> > > > -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> > > > +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> > > >   {
> > > >   	if (!READ_ONCE(sched->pause_submit))
> > > > -		queue_work(sched->submit_wq, &sched->work_submit);
> > > > +		queue_work(sched->submit_wq, &sched->work_run_job);
> > > > +}
> > > > +
> > > > +static struct drm_sched_entity *
> > > > +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
> > > > +
> > > > +/**
> > > > + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> > > > + * @sched: scheduler instance
> > > > + */
> > > > +static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler *sched)
> > > > +{
> > > > +	if (drm_sched_select_entity(sched, false))
> > > > +		drm_sched_run_job_queue(sched);
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_sched_free_job_queue - queue free job
> > > > + *
> > > > + * @sched: scheduler instance to queue free job
> > > > + */
> > > > +static void drm_sched_free_job_queue(struct drm_gpu_scheduler *sched)
> > > > +{
> > > > +	if (!READ_ONCE(sched->pause_submit))
> > > > +		queue_work(sched->submit_wq, &sched->work_free_job);
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_sched_free_job_queue_if_ready - queue free job if ready
> > > > + *
> > > > + * @sched: scheduler instance to queue free job
> > > > + */
> > > > +static void drm_sched_free_job_queue_if_ready(struct drm_gpu_scheduler *sched)
> > > > +{
> > > > +	struct drm_sched_job *job;
> > > > +
> > > > +	spin_lock(&sched->job_list_lock);
> > > > +	job = list_first_entry_or_null(&sched->pending_list,
> > > > +				       struct drm_sched_job, list);
> > > > +	if (job && dma_fence_is_signaled(&job->s_fence->finished))
> > > > +		drm_sched_free_job_queue(sched);
> > > > +	spin_unlock(&sched->job_list_lock);
> > > >   }
> > > >   /**
> > > > @@ -310,7 +359,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
> > > >   	dma_fence_get(&s_fence->finished);
> > > >   	drm_sched_fence_finished(s_fence, result);
> > > >   	dma_fence_put(&s_fence->finished);
> > > > -	drm_sched_submit_queue(sched);
> > > > +	drm_sched_free_job_queue(sched);
> > > >   }
> > > >   /**
> > > > @@ -906,18 +955,19 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
> > > >   void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched)
> > > >   {
> > > >   	if (drm_sched_can_queue(sched))
> > > > -		drm_sched_submit_queue(sched);
> > > > +		drm_sched_run_job_queue(sched);
> > > >   }
> > > >   /**
> > > >    * drm_sched_select_entity - Select next entity to process
> > > >    *
> > > >    * @sched: scheduler instance
> > > > + * @dequeue: dequeue selected entity
> > > >    *
> > > >    * Returns the entity to process or NULL if none are found.
> > > >    */
> > > >   static struct drm_sched_entity *
> > > > -drm_sched_select_entity(struct drm_gpu_scheduler *sched)
> > > > +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue)
> > > >   {
> > > >   	struct drm_sched_entity *entity;
> > > >   	int i;
> > > > @@ -935,8 +985,10 @@ drm_sched_select_entity(struct drm_gpu_scheduler *sched)
> > > >   	/* Kernel run queue has higher priority than normal run queue*/
> > > >   	for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
> > > >   		entity = sched->sched_policy == DRM_SCHED_POLICY_FIFO ?
> > > > -			drm_sched_rq_select_entity_fifo(&sched->sched_rq[i]) :
> > > > -			drm_sched_rq_select_entity_rr(&sched->sched_rq[i]);
> > > > +			drm_sched_rq_select_entity_fifo(&sched->sched_rq[i],
> > > > +							dequeue) :
> > > > +			drm_sched_rq_select_entity_rr(&sched->sched_rq[i],
> > > > +						      dequeue);
> > > >   		if (entity)
> > > >   			break;
> > > >   	}
> > > > @@ -1024,30 +1076,44 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
> > > >   EXPORT_SYMBOL(drm_sched_pick_best);
> > > >   /**
> > > > - * drm_sched_main - main scheduler thread
> > > > + * drm_sched_free_job_work - worker to call free_job
> > > >    *
> > > > - * @param: scheduler instance
> > > > + * @w: free job work
> > > >    */
> > > > -static void drm_sched_main(struct work_struct *w)
> > > > +static void drm_sched_free_job_work(struct work_struct *w)
> > > >   {
> > > >   	struct drm_gpu_scheduler *sched =
> > > > -		container_of(w, struct drm_gpu_scheduler, work_submit);
> > > > -	struct drm_sched_entity *entity;
> > > > +		container_of(w, struct drm_gpu_scheduler, work_free_job);
> > > >   	struct drm_sched_job *cleanup_job;
> > > > -	int r;
> > > >   	if (READ_ONCE(sched->pause_submit))
> > > >   		return;
> > > >   	cleanup_job = drm_sched_get_cleanup_job(sched);
> > > I tried this patch with Nouveau and found a race condition:
> > > 
> > > In drm_sched_run_job_work() the job is added to the pending_list via
> > > drm_sched_job_begin(), then the run_job() callback is called and the scheduled
> > > fence is signaled.
> > > 
> > > However, in parallel drm_sched_get_cleanup_job() might be called from
> > > drm_sched_free_job_work(), which picks the first job from the pending_list and
> > > for the next job on the pending_list sets the scheduled fence' timestamp field.
> 
> Well why can this happen in parallel? Either the work items are scheduled to
> a single threaded work queue or you have protected the pending list with
> some locks.
> 

Xe uses a single-threaded work queue, Nouveau does not (desired
behavior).

The list of pending jobs is protected by a lock (safe), the race is:

add job to pending list
run_job
signal scheduled fence

dequeue from pending list
free_job
update timestamp

Once a job is on the pending list its timestamp can be accessed which
can blow up if scheduled fence isn't signaled or more specifically unless
DMA_FENCE_FLAG_TIMESTAMP_BIT is set. Logically it makes sense for the
job to be in the pending list before run_job and signal the scheduled
fence after run_job so I think we need to live with this race.

> Just moving the free_job into a separate work item without such precautions
> won't work because of quite a bunch of other reasons as well.
>

Yes, free_job might not be safe to run in parallel with run_job
depending on the driver vfuncs. Mention this in the cover letter.

Certainly this should be safe in the scheduler code though and I think
it will be after fixing this.

Matt

> > > 
> > > The job can be on the pending_list, but the scheduled fence might not yet be
> > > signaled. The call to actually signal the fence will subsequently fault because
> > > it will try to dereference the timestamp.
> > > 
> > > I'm not sure what's the best way to fix this, maybe it's enough to re-order
> > > signalling the scheduled fence and adding the job to the pending_list. Not sure
> > > if this has other implications though.
> > > 
> > We really want the job on the pending list before calling run_job.
> > 
> > I'm thinking we just delete the updating of the timestamp, not sure why
> > this is useful.
> 
> This is used for calculating how long each job has spend on the hw, so big
> NAK to deleting this.
>

Ah, I see that AMDGPU uses this. Previously just checked the scheduler
code.

The below patch should work just fine then.

Matt

> Regards,
> Christian.
> 
> > 
> > Or we could do something like this where we try to update the timestamp,
> > if we can't update the timestamp run_job worker will do it anyways.
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 67e0fb6e7d18..54bd3e88f139 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -1074,8 +1074,10 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
> >                                                  typeof(*next), list);
> > 
> >                  if (next) {
> > -                       next->s_fence->scheduled.timestamp =
> > -                               job->s_fence->finished.timestamp;
> > +                       if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> > +                                    &next->s_fence->scheduled.flags))
> > +                               next->s_fence->scheduled.timestamp =
> > +                                       job->s_fence->finished.timestamp;
> >                          /* start TO timer for next job */
> >                          drm_sched_start_timeout(sched);
> >                  }
> > 
> > I guess I'm leaning towards the latter option.
> > 
> > Matt
> > 
> > > - Danilo
> > > 
> > > > -	entity = drm_sched_select_entity(sched);
> > > > +	if (cleanup_job) {
> > > > +		sched->ops->free_job(cleanup_job);
> > > > +
> > > > +		drm_sched_free_job_queue_if_ready(sched);
> > > > +		drm_sched_run_job_queue_if_ready(sched);
> > > > +	}
> > > > +}
> > > > -	if (!entity && !cleanup_job)
> > > > -		return;	/* No more work */
> > > > +/**
> > > > + * drm_sched_run_job_work - worker to call run_job
> > > > + *
> > > > + * @w: run job work
> > > > + */
> > > > +static void drm_sched_run_job_work(struct work_struct *w)
> > > > +{
> > > > +	struct drm_gpu_scheduler *sched =
> > > > +		container_of(w, struct drm_gpu_scheduler, work_run_job);
> > > > +	struct drm_sched_entity *entity;
> > > > +	int r;
> > > > -	if (cleanup_job)
> > > > -		sched->ops->free_job(cleanup_job);
> > > > +	if (READ_ONCE(sched->pause_submit))
> > > > +		return;
> > > > +	entity = drm_sched_select_entity(sched, true);
> > > >   	if (entity) {
> > > >   		struct dma_fence *fence;
> > > >   		struct drm_sched_fence *s_fence;
> > > > @@ -1056,9 +1122,7 @@ static void drm_sched_main(struct work_struct *w)
> > > >   		sched_job = drm_sched_entity_pop_job(entity);
> > > >   		if (!sched_job) {
> > > >   			complete_all(&entity->entity_idle);
> > > > -			if (!cleanup_job)
> > > > -				return;	/* No more work */
> > > > -			goto again;
> > > > +			return;	/* No more work */
> > > >   		}
> > > >   		s_fence = sched_job->s_fence;
> > > > @@ -1088,10 +1152,8 @@ static void drm_sched_main(struct work_struct *w)
> > > >   		}
> > > >   		wake_up(&sched->job_scheduled);
> > > > +		drm_sched_run_job_queue_if_ready(sched);
> > > >   	}
> > > > -
> > > > -again:
> > > > -	drm_sched_submit_queue(sched);
> > > >   }
> > > >   /**
> > > > @@ -1150,7 +1212,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
> > > >   	spin_lock_init(&sched->job_list_lock);
> > > >   	atomic_set(&sched->hw_rq_count, 0);
> > > >   	INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
> > > > -	INIT_WORK(&sched->work_submit, drm_sched_main);
> > > > +	INIT_WORK(&sched->work_run_job, drm_sched_run_job_work);
> > > > +	INIT_WORK(&sched->work_free_job, drm_sched_free_job_work);
> > > >   	atomic_set(&sched->_score, 0);
> > > >   	atomic64_set(&sched->job_id_count, 0);
> > > >   	sched->pause_submit = false;
> > > > @@ -1275,7 +1338,8 @@ EXPORT_SYMBOL(drm_sched_submit_ready);
> > > >   void drm_sched_submit_stop(struct drm_gpu_scheduler *sched)
> > > >   {
> > > >   	WRITE_ONCE(sched->pause_submit, true);
> > > > -	cancel_work_sync(&sched->work_submit);
> > > > +	cancel_work_sync(&sched->work_run_job);
> > > > +	cancel_work_sync(&sched->work_free_job);
> > > >   }
> > > >   EXPORT_SYMBOL(drm_sched_submit_stop);
> > > > @@ -1287,6 +1351,7 @@ EXPORT_SYMBOL(drm_sched_submit_stop);
> > > >   void drm_sched_submit_start(struct drm_gpu_scheduler *sched)
> > > >   {
> > > >   	WRITE_ONCE(sched->pause_submit, false);
> > > > -	queue_work(sched->submit_wq, &sched->work_submit);
> > > > +	queue_work(sched->submit_wq, &sched->work_run_job);
> > > > +	queue_work(sched->submit_wq, &sched->work_free_job);
> > > >   }
> > > >   EXPORT_SYMBOL(drm_sched_submit_start);
> > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > > > index 04eec2d7635f..fbc083a92757 100644
> > > > --- a/include/drm/gpu_scheduler.h
> > > > +++ b/include/drm/gpu_scheduler.h
> > > > @@ -487,9 +487,10 @@ struct drm_sched_backend_ops {
> > > >    *                 finished.
> > > >    * @hw_rq_count: the number of jobs currently in the hardware queue.
> > > >    * @job_id_count: used to assign unique id to the each job.
> > > > - * @submit_wq: workqueue used to queue @work_submit
> > > > + * @submit_wq: workqueue used to queue @work_run_job and @work_free_job
> > > >    * @timeout_wq: workqueue used to queue @work_tdr
> > > > - * @work_submit: schedules jobs and cleans up entities
> > > > + * @work_run_job: schedules jobs
> > > > + * @work_free_job: cleans up jobs
> > > >    * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
> > > >    *            timeout interval is over.
> > > >    * @pending_list: the list of jobs which are currently in the job queue.
> > > > @@ -518,7 +519,8 @@ struct drm_gpu_scheduler {
> > > >   	atomic64_t			job_id_count;
> > > >   	struct workqueue_struct		*submit_wq;
> > > >   	struct workqueue_struct		*timeout_wq;
> > > > -	struct work_struct		work_submit;
> > > > +	struct work_struct		work_run_job;
> > > > +	struct work_struct		work_free_job;
> > > >   	struct delayed_work		work_tdr;
> > > >   	struct list_head		pending_list;
> > > >   	spinlock_t			job_list_lock;
> > > > -- 
> > > > 2.34.1
> > > > 
>