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.br...@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
> > > >
>
