On 04/04/2023 10.58, Matthew Brost wrote:
On Tue, Apr 04, 2023 at 10:07:48AM +0900, Asahi Lina wrote:
Hi, thanks for the Cc!
No problem.
On 04/04/2023 09.22, Matthew Brost wrote:
Hello,
As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
have been asked to merge our common DRM scheduler patches first as well
as develop a common solution for long running workloads with the DRM
scheduler. This RFC series is our first attempt at doing this. We
welcome any and all feedback.
This can we thought of as 4 parts detailed below.
- DRM scheduler changes for 1 to 1 relationship between scheduler and
entity (patches 1-3)
In Xe all of the scheduling of jobs is done by a firmware scheduler (the
GuC) which is a new paradigm WRT to the DRM scheduler and presents
severals problems as the DRM was originally designed to schedule jobs on
hardware queues. The main problem being that DRM scheduler expects the
submission order of jobs to be the completion order of jobs even across
multiple entities. This assumption falls apart with a firmware scheduler
as a firmware scheduler has no concept of jobs and jobs can complete out
of order. A novel solution for was originally thought of by Faith during
the initial prototype of Xe, create a 1 to 1 relationship between scheduler
and entity. I believe the AGX driver [3] is using this approach and
Boris may use approach as well for the Mali driver [4].
To support a 1 to 1 relationship we move the main execution function
from a kthread to a work queue and add a new scheduling mode which
bypasses code in the DRM which isn't needed in a 1 to 1 relationship.
The new scheduling mode should unify all drivers usage with a 1 to 1
relationship and can be thought of as using scheduler as a dependency /
infligt job tracker rather than a true scheduler.
Yup, we're in the exact same situation with drm/asahi, so this is very
welcome! We've been using the existing scheduler as-is, but this should help
remove some unneeded complexity in this use case.
That's the idea.
Do you want me to pull in this series into our tree and make sure this all
works out for us?
We tested this in Xe and it definitely works for us but the more testing
the better.
I haven't gotten around to testing this series yet, but after more
debugging of drm_sched issues I want to hear more about how Xe uses the
scheduler.
From what I can tell, and from what Christian says, drm_sched has the
hidden requirement that all job objects outlive the scheduler. I've run
into several UAF bugs due to this. Not only that, it also currently has
the requirement that all drm_sched fences outlive the scheduler object.
These requirements are subtle and only manifest as kernel oopses in rare
corner cases, so it wasn't at all obvious to me that this was somehow a
fundamental design assumption when I started using it.
As far as I can tell, this design is going to work in 99% of cases for
global-schedulers-per-GPU models, where those corner cases would have to
be hit on top of a GPU removal scenario (and GPU remove is... well, not
the most tested/exercised use case). When the scheduler basically lives
forever, none of this really matters.
But with a one-scheduler-per-queue model, how do you deal with this when
the queue goes away? So far, without any of the partial bugfixes I have
sent so far (which Christian objected to):
- If you try to tear down a scheduler with any jobs currently scheduled
at the hardware, drm_sched will oops when those jobs complete and the hw
fences signal.
- If you try to tear down an entity (which should cancel all its pending
jobs) and then the scheduler it was attached to without actually waiting
for all the free_job() callbacks to be called on every job that ever
existed for that entity, you can oops (entity cleanup is asynchronous in
some cases like killed processes, so it will return before all jobs are
freed and then that asynchronous process will crash and burn if the
scheduler goes away out from under its feet). Waiting for job completion
fences is not enough for this, you have to wait until free_job() has
actually been called for all jobs.
- Even if you actually wait for all jobs to be truly gone and then tear
down the scheduler, if any scheduler job fences remain alive, that will
then oops if you try to call the debug functions on them (like cat
/sys/kernel/debug/dma_buf/bufinfo).
I tried to fix these things, but Christian objected implying it was the
driver's job to keep a reference from jobs and hw fences to the
scheduler. But I find that completely broken, because besides the extra
memory/resource usage keeping the scheduler alive when you're trying to
free resources as fast as possible when a process goes away, you can't
even use normal reference counting for that: if you try to drop the last
drm_sched reference from within a free_job() callback, the whole thing
deadlocks since that will be running in the scheduler's thread/workqueue
context, which can't free itself. So now you both reference count the
scheduler from jobs and fences, and on top of that you need to outsource
drm_sched freeing to a workqueue in the driver to make sure you don't
deadlock.
For job fences this is particularly broken, because those fences can
live forever signaled and attached to shared buffers and there is no
guarantee that they will be freed in any kind of reasonable time frame.
If they have to keep the scheduler that created them alive, that creates
a lot of dead object junk we have to drag around just because a signaled
fence exists somewhere.
For a Rust abstraction we have to do all that tracking and refcounting
in the abstraction itself to make it safe, which is starting to sound
like reimplementing half of the job tracking drm_sched itself does just
to fix the lifetime issues, which really tells me the existing design is
not sound nor easy to use correctly in general.
How does Xe deal with this (does it deal with it at all)? What happens
when you kill -9 a process using the GPU? Does freeing all of this wait
for all jobs to complete *and be freed* with free_job()? What about
exported dma_bufs with fences attached from that scheduler? Do you keep
the scheduler alive for those?
Personally, after running into all this, and after seeing Christian's
reaction to me trying to improve the state of things, I'm starting to
doubt that drm_sched is the right solution at all for
firmware-scheduling drivers.
If you want a workload to try to see if you run into any of these
things, running and killing lots of things in parallel is a good thing
to try (mess with the numbers and let it run for a while to see if you
can hit any corner cases):
while true; do for i in $(seq 1 10); do timeout -k 0.01 0.05 glxgears &
done; sleep 0.1; done
~~ Lina
