Re: [RFC v4 12/16] drm/sched: Remove idle entity from tree

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 12/05/2025 14:03, Philipp Stanner wrote:
> On Fri, 2025-04-25 at 11:20 +0100, Tvrtko Ursulin wrote:
>> There is no need to keep entities with no jobs in the tree so lets
>> remove
>> it once the last job is consumed. This keeps the tree smaller which
>> is
>> nicer and more efficient as entities are removed and re-added on
>> every
>> popped job.
> 
> That there is no need to do so doesn't imply that you can't keep them
> around. The commit message doesn't make the motivation clear
> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> ---
>>   drivers/gpu/drm/scheduler/sched_rq.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> Since this doesn't simplify the code base, I think the only
> justification would be a somewhat decent performance gain. Does this
> patch result in that?
> 
> Otherwise it's probably better to keep git-blame intact here.

I needed this for one of the earlier approaches and I *think* what 
remains with the latest is just the fact it makes the run-queue contain 
only runnable entities (which makes sense and is logical; run-queue <-> 
runnable). And that rb-tree re-balancing is cheaper with smaller trees 
but in the grand scheme of things it is not something I even considered 
attempting to measure.

I will re-consider the fate of this patch once more feedback on the 
series as overall is received. Until then I don't think it makes sense 
to churn it.

Btw another angle to this, which we touched upon with Christian before 
is, if we end up not pruning the tree from unrunnable entities, then we 
could drop the drm_sched_rq->entities list. Making a handful of caller 
which walk it walk the tree instead.

Regards,

Tvrtko

>> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c
>> b/drivers/gpu/drm/scheduler/sched_rq.c
>> index d477a027feb9..2cde89cf25fb 100644
>> --- a/drivers/gpu/drm/scheduler/sched_rq.c
>> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
>> @@ -149,25 +149,27 @@ void drm_sched_rq_pop_entity(struct
>> drm_sched_entity *entity)
>>   {
>>   	struct drm_sched_job *next_job;
>>   	struct drm_sched_rq *rq;
>> -	ktime_t ts;
>>   
>>   	/*
>>   	 * Update the entity's location in the min heap according to
>>   	 * the timestamp of the next job, if any.
>>   	 */
>> +	spin_lock(&entity->lock);
>> +	rq = entity->rq;
>> +	spin_lock(&rq->lock);
>>   	next_job = drm_sched_entity_queue_peek(entity);
>> -	if (!next_job)
>> -		return;
>> +	if (next_job) {
>> +		ktime_t ts;
>>   
>> -	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> -		ts = next_job->submit_ts;
>> -	else
>> -		ts = drm_sched_rq_get_rr_deadline(rq);
>> +		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> +			ts = next_job->submit_ts;
>> +		else
>> +			ts = drm_sched_rq_get_rr_deadline(rq);
>>   
>> -	spin_lock(&entity->lock);
>> -	rq = entity->rq;
>> -	spin_lock(&rq->lock);
>> -	drm_sched_rq_update_fifo_locked(entity, rq, ts);
>> +		drm_sched_rq_update_fifo_locked(entity, rq, ts);
>> +	} else {
>> +		drm_sched_rq_remove_fifo_locked(entity, rq);
>> +	}
>>   	spin_unlock(&rq->lock);
>>   	spin_unlock(&entity->lock);
>>   }
>