On 11/09/2026 09:56, Philipp Stanner wrote:
On Mon, 2026-09-07 at 14:05 +0100, Tvrtko Ursulin wrote:
Prevent pushing a new job to an entity seeing it being the first in the
queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
side in drm_sched_entity_pop_job() has de-queued the job but not yet
updated the saved virtual time.
I think I don't fully get the bug from that message. The bug (is it
even one?) you address is the performance bottleneck for CFS under high
load, right?
Not high load but a race condition between the save and restore of
virtual time when entities enter/leave a run queue.
Anyways, I think I got a better idea:
We do this by pulling the locked sections out to encompass both the queue
push/pop and corresponding rbtree management.
So basically you move for locking the spsc-queue, the lockless queue
which is usually used with the locks next to it anyways. That's cool
stuff. I've suggested this for a while already
https://docs.kernel.org/gpu/todo.html#replace-the-lockless-queue-with-a-locked-list
So from a maintainability perspective, it would be far cooler if you go
down this road one step further and kill the spsc_queue for good,
replacing it with a fully locked list. AFAIR Christian and Danilo also
have agreed to this.
I am happy to work on that just that a) I thought some time back you
said someone from RedHat will tackle it, and b) lets please not gate
this bugfix on that.
[…]
@@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
*/
void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
{
+ struct drm_sched_rq *rq = entity->rq;
struct drm_sched_job *next_job;
- struct drm_sched_rq *rq;
+
+ lockdep_assert_held(&entity->lock);
btw. I think it *reads* a bit safer if the rq initalization happens
below the lockdep assertion. Also might make the diff nicer.
It makes not practical difference but sure, I agree it reads better so I
can respin once you clarify if you are blocking the bugfix until the
spsc removal or what.
Regards,
Tvrtko
