[PATCH 8/8] drm/sched: Further optimise drm_sched_entity_push_job

[Christian König]

Am 10.09.24 um 11:46 schrieb Tvrtko Ursulin:
>
> On 10/09/2024 10:08, Christian König wrote:
>> Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>
>>> Having removed one re-lock cycle on the entity->lock in a patch titled
>>> "drm/sched: Optimise drm_sched_entity_push_job", with only a tiny bit
>>> larger refactoring we can do the same optimisation on the rq->lock.
>>> (Currently both drm_sched_rq_add_entity() and
>>> drm_sched_rq_update_fifo_locked() take and release the same lock.)
>>
>> I think that goes into the wrong direction.
>>
>> Probably better to move this here into drm_sched_rq_add_entity():
>>
>>            if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>               drm_sched_rq_update_fifo_locked(entity, submit_ts);
>>
>> We can then also drop adding the entity to the rr list when FIFO is 
>> in use.
>
> Unfortuntely there is a few other places which appear to rely on the 
> list. Like drm_sched_fini,

That should be only a warning.

> drm_sched_increase_karma and 

The karma handling was another bad idea from AMD how to populate back 
errors to userspace and I've just recently documented together with Sima 
that we should use dma-fence errors instead.

Just didn't had time to tackle cleaning that up yet.

> even amdgpu_job_stop_all_jobs_on_sched.

Uff, seeing that for the first time just now. Another bad idea how to 
handle things which doesn't take the appropriate locks and looks racy to me.


> Latter could perhaps be solved by adding an iterator helper to the 
> scheduler, which would perhaps be a good move for component isolation. 
> And first two could be handled by implementing a complete and mutually 
> exclusive duality of how entities are walked depending on scheduling 
> mode. Plus making the scheduling mode only be configurable at boot. It 
> feels doable but significant work and in the meantime removing the 
> double re-lock maybe acceptable?

I don't think we should optimize for something we want to remove in the 
long term.

If possible I would rather say that we should completely drop the RR 
approach and only use FIFO or even something more sophisticated.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>>>
>>> To achieve this we rename drm_sched_rq_add_entity() to
>>> drm_sched_rq_add_entity_locked(), making it expect the rq->lock to be
>>> held, and also add the same expectation to
>>> drm_sched_rq_update_fifo_locked().
>>>
>>> For more stream-lining we also add the run-queue as an explicit 
>>> parameter
>>> to drm_sched_rq_remove_fifo_locked() to avoid both callers and callee
>>> having to dereference entity->rq.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>> Cc: Luben Tuikov <ltuikov89 at gmail.com>
>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>> Cc: Philipp Stanner <pstanner at redhat.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c |  7 ++--
>>>   drivers/gpu/drm/scheduler/sched_main.c   | 41 
>>> +++++++++++++-----------
>>>   include/drm/gpu_scheduler.h              |  7 ++--
>>>   3 files changed, 31 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index b4c4f9923e0b..2102c726d275 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -614,11 +614,14 @@ void drm_sched_entity_push_job(struct 
>>> drm_sched_job *sched_job)
>>>           sched = rq->sched;
>>>           atomic_inc(sched->score);
>>> -        drm_sched_rq_add_entity(rq, entity);
>>> +
>>> +        spin_lock(&rq->lock);
>>> +        drm_sched_rq_add_entity_locked(rq, entity);
>>>           if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>> -            drm_sched_rq_update_fifo_locked(entity, submit_ts);
>>> +            drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
>>> +        spin_unlock(&rq->lock);
>>>           spin_unlock(&entity->lock);
>>>           drm_sched_wakeup(sched, entity);
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 937e7d1cfc49..1ccd2aed2d32 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -153,41 +153,44 @@ static __always_inline bool 
>>> drm_sched_entity_compare_before(struct rb_node *a,
>>>       return ktime_before(ent_a->oldest_job_waiting, 
>>> ent_b->oldest_job_waiting);
>>>   }
>>> -static inline void drm_sched_rq_remove_fifo_locked(struct 
>>> drm_sched_entity *entity)
>>> +static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity 
>>> *entity,
>>> +                        struct drm_sched_rq *rq)
>>>   {
>>> -    struct drm_sched_rq *rq = entity->rq;
>>> -
>>>       if (!RB_EMPTY_NODE(&entity->rb_tree_node)) {
>>>           rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
>>>           RB_CLEAR_NODE(&entity->rb_tree_node);
>>>       }
>>>   }
>>> -void drm_sched_rq_update_fifo_locked(struct drm_sched_entity 
>>> *entity, ktime_t ts)
>>> +void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>>> +                     struct drm_sched_rq *rq,
>>> +                     ktime_t ts)
>>>   {
>>>       lockdep_assert_held(&entity->lock);
>>> +    lockdep_assert_held(&rq->lock);
>>> -    spin_lock(&entity->rq->lock);
>>> -
>>> -    drm_sched_rq_remove_fifo_locked(entity);
>>> +    drm_sched_rq_remove_fifo_locked(entity, rq);
>>>       entity->oldest_job_waiting = ts;
>>> -    rb_add_cached(&entity->rb_tree_node, &entity->rq->rb_tree_root,
>>> +    rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>>>                 drm_sched_entity_compare_before);
>>> -
>>> -    spin_unlock(&entity->rq->lock);
>>>   }
>>>   void drm_sched_rq_update_fifo(struct drm_sched_entity *entity, 
>>> ktime_t ts)
>>>   {
>>> +    struct drm_sched_rq *rq;
>>> +
>>>       /*
>>>        * Both locks need to be grabbed, one to protect from 
>>> entity->rq change
>>>        * for entity from within concurrent 
>>> drm_sched_entity_select_rq and the
>>>        * other to update the rb tree structure.
>>>        */
>>>       spin_lock(&entity->lock);
>>> -    drm_sched_rq_update_fifo_locked(entity, ts);
>>> +    rq = entity->rq;
>>> +    spin_lock(&rq->lock);
>>> +    drm_sched_rq_update_fifo_locked(entity, rq, ts);
>>> +    spin_unlock(&rq->lock);
>>>       spin_unlock(&entity->lock);
>>>   }
>>> @@ -210,25 +213,23 @@ static void drm_sched_rq_init(struct 
>>> drm_gpu_scheduler *sched,
>>>   }
>>>   /**
>>> - * drm_sched_rq_add_entity - add an entity
>>> + * drm_sched_rq_add_entity_locked - add an entity
>>>    *
>>>    * @rq: scheduler run queue
>>>    * @entity: scheduler entity
>>>    *
>>>    * Adds a scheduler entity to the run queue.
>>>    */
>>> -void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>> -                 struct drm_sched_entity *entity)
>>> +void drm_sched_rq_add_entity_locked(struct drm_sched_rq *rq,
>>> +                    struct drm_sched_entity *entity)
>>>   {
>>> +    lockdep_assert_held(&rq->lock);
>>> +
>>>       if (!list_empty(&entity->list))
>>>           return;
>>> -    spin_lock(&rq->lock);
>>> -
>>>       atomic_inc(rq->sched->score);
>>>       list_add_tail(&entity->list, &rq->entities);
>>> -
>>> -    spin_unlock(&rq->lock);
>>>   }
>>>   /**
>>> @@ -242,6 +243,8 @@ void drm_sched_rq_add_entity(struct drm_sched_rq 
>>> *rq,
>>>   void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>>>                   struct drm_sched_entity *entity)
>>>   {
>>> +    lockdep_assert_held(&entity->lock);
>>> +
>>>       if (list_empty(&entity->list))
>>>           return;
>>> @@ -254,7 +257,7 @@ void drm_sched_rq_remove_entity(struct 
>>> drm_sched_rq *rq,
>>>           rq->current_entity = NULL;
>>>       if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>> -        drm_sched_rq_remove_fifo_locked(entity);
>>> +        drm_sched_rq_remove_fifo_locked(entity, rq);
>>>       spin_unlock(&rq->lock);
>>>   }
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 5a1e4c803b90..2ad33e2fe2d2 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -591,13 +591,14 @@ bool drm_sched_dependency_optimized(struct 
>>> dma_fence* fence,
>>>                       struct drm_sched_entity *entity);
>>>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>>> -void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>> -                 struct drm_sched_entity *entity);
>>> +void drm_sched_rq_add_entity_locked(struct drm_sched_rq *rq,
>>> +                    struct drm_sched_entity *entity);
>>>   void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>>>                   struct drm_sched_entity *entity);
>>>   void drm_sched_rq_update_fifo(struct drm_sched_entity *entity, 
>>> ktime_t ts);
>>> -void drm_sched_rq_update_fifo_locked(struct drm_sched_entity 
>>> *entity, ktime_t ts);
>>> +void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>>> +                     struct drm_sched_rq *rq, ktime_t ts);
>>>   int drm_sched_entity_init(struct drm_sched_entity *entity,
>>>                 enum drm_sched_priority priority,
>>