Re: [PATCH] drm/sched: Do not restore unsaved virtual runtime

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 11, 2026 at 10:18:29AM +0100, Tvrtko Ursulin wrote:
> 
> 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.

No. This style is used throughout the kernel. Your suggestion makes the
code harder to read, and it scales poorly as the number of stack
variables increases.

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

Yes. The lockdep invarient holds regardless.

Matt

> 
> Regards,
> 
> Tvrtko