On 24/09/2025 10:15, Philipp Stanner wrote:
On Thu, 2025-09-11 at 16:06 +0100, Tvrtko Ursulin wrote:
On 11/09/2025 15:32, Philipp Stanner wrote:
On Wed, 2025-09-03 at 11:18 +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.
This reads suspiciously as if it could be an independent patch, not
necessarily tied to this series. I see it depends on the _pop()
function you added.
I think you'd want to make it a bit more obvious that this is not so
much a general improvement as it is a preparation for followup work. Or
could it be made generic for the current in-tree scheduler?
Both is true. There is currently no reason idle entities _need_ to be in
the tree. Removing them would improve O(log n) on the rbtree. But also
fair scheduler relies on it, see below...
Apart from that, the upcoming fair scheduling algorithm will rely on the
tree only containing runnable entities.
... ^^^ here.
Yes, I saw that.
I wanted to stress where I'm coming from: generic code improvements
should ideally be posted as separate patches, because that makes it
easier to review and quicker to merge (and easier to revert should a
problem be detected before the subsequent CFS series is merged)
So, can you submit this patch separately without too much effort? :)
It actually started as the 2nd patch in the series back in Dec/24, but
as over the past period no one was convinced on its worth on its own, I
kept moving it to later and later in the series. Up to the point where
the series actually depends on it.
I think at this point it is better if it stays where it is. Because
without the fair scheduler the benefit is harder to justify. Yes it
reduces the rbtree rebalancing cost, but importance of that only
increases when there are a lot of open DRM nodes, or clients with a lot
of entities. On the other side it adds a spin lock to the job pop flow
and I did not measure the effect on either scenario. At least with the
fair scheduler the benefit is clear.
Regards,
Tvrtko
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Christian König <[email protected]>
Cc: Danilo Krummrich <[email protected]>
Cc: Matthew Brost <[email protected]>
Cc: Philipp Stanner <[email protected]>
---
drivers/gpu/drm/scheduler/sched_rq.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c
b/drivers/gpu/drm/scheduler/sched_rq.c
index 16d57461765e..67804815ca0d 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -19,6 +19,9 @@ drm_sched_entity_compare_before(struct rb_node *a, const
struct rb_node *b)
static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
struct drm_sched_rq *rq)
{
+ lockdep_assert_held(&entity->lock);
+ lockdep_assert_held(&rq->lock);
The callers of drm_sched_rq_remove_fifo_locked() already have some
lockdep asserts, have you seen them? Are those here redundant /
additional?
And are they strictly related to this patch?
Yes, drm_sched_rq_pop_entity() is the new caller, which needs to take
both locks on its own. So IMO makes sense to add the asserts.
Maybe you want to investigate the other lockdep assertions and, if
there's room for improvement, address that in a dedicated patch.
They look okay to me. Are you seeing something is off?
No, they do look correct. It's just that we have a bit of redundancy
then, but that's probably a good thing for robustness.
P.
Regards,
Tvrtko
+
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);
@@ -158,24 +161,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;
- spin_lock(&entity->lock);
- rq = entity->rq;
- spin_lock(&rq->lock);
- if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
- ts = next_job->submit_ts;
- else
- ts = drm_sched_rq_get_rr_ts(rq, entity);
- drm_sched_rq_update_fifo_locked(entity, rq, ts);
+ if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+ ts = next_job->submit_ts;
+ else
+ ts = drm_sched_rq_get_rr_ts(rq, entity);
+
+ 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);
}
