On Sat, Apr 08, 2023 at 04:05:20PM +0900, Asahi Lina wrote: > 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. Bit a wash-up reply on the more fundamental thing here: For the current scheduler the issues you've found are indeed all driver bugs (or most I think at least). Which is why I think we shouldn't just try to shoehorn fundamentally new semantics without updating the driver interfaces (the drm_sched split into the driver interface part and the internal scheduler part). Once we have that, including kerneldoc update and what the rules are, then all the various uaf you've discovered become real bugs and I don't see any issue merging all the fixes. Without that we do have a chicken/egg problem between: "here's a bunch of hacks to make the problems disappear I've hit in my reuse of drm/sched for fw schedulers" vs. "this makes no sense for the current drm/sched interfaces and how current upstream drivers use it" I don't think there's a lot needed in terms of drm/sched driver api rework, but I think it's also pretty clearly not ever going to get anywhere with just nothing at all. Writing an entire new scheduler lib instead of at least trying what minimal semantic changes (instead of just a pile of hacks without even doc changes for the new rules) does not sound like a good idea to me :-) > 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 Maybe xe gets away with this due to synchronously killing everything related to a ctx, but yeah I'd expect this to go boom in fun ways. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
