Re: [RFC PATCH v5 0/6] rust: Add drm::JobQueue

[Philipp Stanner <phasta@mailbox.org>]

+Cc Janne

On Sat, 2026-10-10 at 21:22 -0300, Daniel Almeida wrote:
> Hi Philipp,
> 
> > On 9 Oct 2026, at 16:11, Philipp Stanner <phasta@kernel.org> wrote:
> > 
> > Changes in v5:
> >  - Add back the Revocable<JobQueueInner…> from my earliest RFCs. Reason
> >    is still the same: JQ dropping while dependency fences signal could
> >    deadlock. Similarly, this solves a dependency kicking of the work
> >    item again while drop() is running.
> 
> Can you expand a bit on this? IIUC the problem is introduced by trying to take
> the jq lock on signal? If so, this could be avoided by queuing the worker from
> that path instead?

That's a good point, but I'm not entirely sure if it would result in
well-defined behavior in Rust. Let's say we design it like this:

struct JobQueue {
  inner: Lock<JobQueueInner<…>>, // <- contains job lists with dep wakers who want to start 'work'
  work: ScopedWork, // <- tries to access 'inner'
}

Now:

JQ drops -> jq.inner drops -> takes lock -> iterates over jobs,
deregistering all dep fences

Simultaneously, a job that the iteration has not yet reached gets
ready, schedules 'work'. The Workqueue backend runs 'work' right away,
it starts spinning on Lock<JobQueueInner>

Dropping of jq.inner releases the Lock around inner. Thus, now the work
item can take the lock, right when 'work' is about to drop and, thus,
before it could be canceled.

So this would be UAF, wouldn't it? Correct me if I'm wrong.

I still use the system_wq (will be changed, TODO). Could we solve the
problem with the right drop order if we have a distinct wq?

> 
> >  - Do not call run_job() with a lock held, since run_job() might sleep.
> >    AFAICS, this also makes the proposed XArray solution impossible and
> >    demands that we use a list.
> 
> Can you expand on this a bit?

That run_job() might sleep was AFAIK proposed in our workshop at LPC. I
think it can't allocate because of the fence critical section path,
though.

Anyways, we have to borrow a job to the driver in run_job(), but
simultaneously we cannot / should not hold the lock of the data
structure (no matter whether XArray or list or ringbuf) in which the
job resides.

So what you effectively have to do is take $DATA_STRUCTURE's lock, pull
out the job, release the lock, call run_job() to borrow the job, and
after running take the lock again to update the data structure.

AFAIU this issue was what you were describing in our workshop when you
were trying to protect xarray's tip-job with a distinct lock?


Note in my last patch that this opens up a race path with
jq.complete_jobs() which we want to address, but that is solvable.

> 
> >  - Re-add the earlier credit-count-system, so that run_job() is
> >    infallible. Simplifies the design quite a bit. (Danilo)
> 
> What is AGX’s position on this? IIRC, this credit system could not represent
> their submission model exactly, like it can for others.

Let's ask them :)
@Janne, WDYT?

[…]

> > 1.
> > I believe that callers of jq.new_job() and jq.submit_job() do not need
> > an outer serialization mutex. Reason is that JQ serializes with its
> > internal lock. Sequence numbers cannot take over older seqnos because it
> > is submit_job() who sets the seqno.
> 
> I guess this makes sense, so long as submit_job() doesn’t take &mut self?

It can be &self now. Earlier we wanted `&mut self` to enforce locking
by the driver, but as I said I don't see a reason for that anymore.

> > 

[…]

> > 2.
> > I, furthermore, believe that a driver will not need to hold a
> > driver-lock while calling jq.complete_jobs_up_to_seqno().
> 
> In tyr, we hold _no_ locks.
> 

Huh? I understood that very differently at LPC. My understanding was
that you had to work hard to get around locking issues between driver-
lock and JQ-lock.


>  The driver signals the fences, and this will
> enqueue the worker thread.

That is not our intended design anymore, because we want the Job to
hold the DriverFence, the JobQueue to hold the FenceContext, and the
JobQueue to hold the jobs.

Thereby, the DriverFence's data: T is also the Job's data: T, which IMO
is the only thing that makes sense, because the job's data is what will
be relevant for the fence backend_ops, and, therefore, is what needs to
be protected by the RCU grace period (see above).

The consequence is that jobs are to be completed with sth like

JobQueue::complete_jobs_up_to(seqno: u64, state: Result)


> 

Thanks for your input, appreciated!

Philipp