On Fri, Nov 22, 2024 at 05:24:03PM +0100, Philipp Stanner wrote: > On Thu, 2024-11-21 at 10:05 -0800, Matthew Brost wrote: > > On Tue, Oct 29, 2024 at 08:22:22AM +0100, Philipp Stanner wrote: > > > Christian, Sima? > > > > > > Matthew? (+CC) > > > > > > > Sorry catching up here. Internally in Xe we ref count the scheduler > > as > > the scheduler is embedded into our user queue structure > > (xe_exec_queue). > > Jobs ref the queue once they are armed. Upon queue killing we set TDR > > to > > zero which will flush out all jobs / signal all the job's fences. > > Once > > the ref count of queue is zero we do hardware / firmware teardown of > > the > > queue and then finally call drm_sched_fini before freeing the queues > > memory (and thus the scheduler). > > > > This flow seems to work pretty well but largely bypasses the > > scheduler > > functions to implement this. Not sure if this flow could be > > normalized > > at all but I would expect usage models of a 1 to 1 relationship > > between > > queue and scheduler to something similar to Xe's flow. > > One of the solution I proposed basically does the same: jobs added to > gpu_scheduler.pending_list refcount the scheduler. > > If we'd go for that solution I assume the separate refcounting could be > removed from Xe. For exmaple. > That should work. We likely need a vfunc for when the refcount goes to zero which could either free the scheduler or initiate hw / hw teardown process. In Xe the queue and scheduler actually lives for a while after the refcount is zero due to several ping / pongs with the fw. Also long as we can still support that, moving the refcount from our queues to the scheduler seems reasonable. Ofc we need buy in from the other scheduler use cases too. > > > Maybe we could > > write this done as design guideline so other drivers don't have to > > figure this out - this took me a bit to land on this. > > I already improved the documentation a while ago [1], but didn't detail > how drivers are supposed to solve this. The reason is that I don't want > to encourage more separate solutions while we're working on solving it > centrally. Let me read this and add comments there if needed. Matt > > > With that, in general I agree with Christian's patch here complaining > > about pending jobs if drm_sched_fini is called. > > Yes, firing a WARN_ON there is also fine from my POV. > > P. > > > [1] https://lore.kernel.org/dri-devel/20241105143137.71893-2-pstanner@xxxxxxxxxx/ > > > > > > > Opinions on the below? > > > > > > tl;dr: > > > I still think it's a good thing to detectably block in > > > drm_sched_fini(), or at the very least drm_sched_flush(), because > > > then > > > > See above. I don't think drm_sched_fini should block rather just > > complain this patch does which says 'go fix your driver'. > > > > Matt > > > > > you'll find out that the driver is broken and can repair it. > > > > > > P. > > > > > > > > > On Fri, 2024-10-18 at 14:07 +0200, Philipp Stanner wrote: > > > > On Mon, 2024-10-14 at 16:56 +0200, Danilo Krummrich wrote: > > > > > On Fri, Sep 27, 2024 at 11:04:48AM +0200, Christian König > > > > > wrote: > > > > > > Am 25.09.24 um 16:53 schrieb Philipp Stanner: > > > > > > > On Tue, 2024-09-24 at 13:18 +0200, Simona Vetter wrote: > > > > > > > > On Mon, Sep 23, 2024 at 05:24:10PM +0200, Christian König > > > > > > > > wrote: > > > > > > > > > Am 20.09.24 um 15:26 schrieb Philipp Stanner: > > > > > > > > > > On Fri, 2024-09-20 at 12:33 +0200, Christian König > > > > > > > > > > wrote: > > > > > > > > > > > Am 20.09.24 um 10:57 schrieb Philipp Stanner: > > > > > > > > > > > > On Wed, 2024-09-18 at 15:39 +0200, Christian > > > > > > > > > > > > König > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Tearing down the scheduler with jobs still on > > > > > > > > > > > > > the > > > > > > > > > > > > > pending > > > > > > > > > > > > > list > > > > > > > > > > > > > can > > > > > > > > > > > > > lead to use after free issues. Add a warning if > > > > > > > > > > > > > drivers try > > > > > > > > > > > > > to > > > > > > > > > > > > > destroy a scheduler which still has work pushed > > > > > > > > > > > > > to > > > > > > > > > > > > > the HW. > > > > > > > > > > > > Did you have time yet to look into my proposed > > > > > > > > > > > > waitque- > > > > > > > > > > > > solution? > > > > > > > > > > > I don't remember seeing anything. What have I > > > > > > > > > > > missed? > > > > > > > > > > https://lore.kernel.org/all/20240903094446.29797-2-pstanner@xxxxxxxxxx/ > > > > > > > > > Mhm, I didn't got that in my inbox for some reason. > > > > > > > > > > > > > > > > > > Interesting approach, I'm just not sure if we can or > > > > > > > > > should > > > > > > > > > wait in > > > > > > > > > drm_sched_fini(). > > > > > > > We do agree that jobs still pending when drm_sched_fini() > > > > > > > starts > > > > > > > is > > > > > > > always a bug, right? > > > > > > > > > > > > Correct, the question is how to avoid that. > > > > > > > > > > > > > If so, what are the disadvantages of waiting in > > > > > > > drm_sched_fini()? > > > > > > > We > > > > > > > could block buggy drivers as I see it. Which wouldn't be > > > > > > > good, > > > > > > > but > > > > > > > could then be fixed on drivers' site. > > > > > > > > > > > > Sima explained that pretty well: Don't block in fops->close, > > > > > > do > > > > > > that in > > > > > > fops->flush instead. > > > > > > > > > > I agree that we shouldn't block in close(), but this > > > > > effectively > > > > > means that we > > > > > need to reference count the scheduler, right? > > > > > > > > > > Otherwise, if we allow to just skip / interrupt the teardown, > > > > > we > > > > > can > > > > > still leak > > > > > memory. > > > > > > > > Having thought about it, I agree with Danilo. Having something > > > > that > > > > shall wait on green light, but can be interrupted, is no > > > > guarantee > > > > and > > > > therefore not a feasible solution. > > > > > > > > To break down the solution space, these seem to be our options: > > > > 1. We have something (either drm_sched_fini() or a helper, > > > > e.g., > > > > drm_sched_flush()) that definitely blocks until the pending > > > > list > > > > has become empty. > > > > 2. We have jobs reference-count the scheduler, so the latter > > > > can > > > > outlive the driver and will be freed some time later. > > > > > > > > Can anyone think of a third solution? > > > > > > > > > > > > Solution #1 has the problem of obviously blocking unconditionally > > > > if > > > > the driver didn't make sure that all fences will be signaled > > > > within > > > > reasonable time. In my opinion, this would actually be an > > > > advantage, > > > > because it will be *very* noticable and force users to repair > > > > their > > > > driver. The driver *has* to guarantee that all fences will be > > > > signaled. > > > > If the driver has to do fishy things, having the blocking > > > > outsourced > > > > to > > > > the helper drm_sched_flush() would allow them to circumvent that. > > > > > > > > Solution #2 has the problem of backend_ops.free_job() potentially > > > > using > > > > driver-data after the driver is gone, causing UAF. So with this > > > > solutions all drivers would have to be aware of the issue and > > > > handle > > > > it > > > > through one of DRMs primitives dedicated to such problems. > > > > > > > > > > > > Currently, all drivers either work around the problem internally > > > > or > > > > simply ignore it, it seems. > > > > > > > > So I'd argue that both solutions are an improvement over the > > > > existing > > > > situation. My preference would be #1. > > > > > > > > > > > > Opinions? > > > > > > > > P. > > > > > > > > > > > > > > > > > > > > > One issue this solves is that when you send a SIGTERM the > > > > > > tear > > > > > > down > > > > > > handling > > > > > > first flushes all the FDs and then closes them. > > > > > > > > > > > > So if flushing the FDs blocks because the process initiated > > > > > > sending > > > > > > a > > > > > > terabyte of data over a 300bps line (for example) you can > > > > > > still > > > > > > throw a > > > > > > SIGKILL and abort that as well. > > > > > > > > > > > > If you would block in fops-close() that SIGKILL won't have > > > > > > any > > > > > > effect any > > > > > > more because by the time close() is called the process is > > > > > > gone > > > > > > and > > > > > > signals > > > > > > are already blocked. > > > > > > > > > > > > And yes when I learned about that issue I was also buffed > > > > > > that > > > > > > handling like > > > > > > this in the UNIX design is nearly 50 years old and still > > > > > > applies > > > > > > to > > > > > > today. > > > > > > > > > Probably better to make that a separate function, > > > > > > > > > something > > > > > > > > > like > > > > > > > > > drm_sched_flush() or similar. > > > > > > > We could do that. Such a function could then be called by > > > > > > > drivers > > > > > > > which > > > > > > > are not sure whether all jobs are done before they start > > > > > > > tearing > > > > > > > down. > > > > > > > > > > > > Yes exactly that's the idea. And give that flush function a > > > > > > return > > > > > > code so > > > > > > that it can return -EINTR. > > > > > > > > > > > > > > Yeah I don't think we should smash this into > > > > > > > > drm_sched_fini > > > > > > > > unconditionally. I think conceptually there's about three > > > > > > > > cases: > > > > > > > > > > > > > > > > - Ringbuffer schedules. Probably want everything as-is, > > > > > > > > because > > > > > > > > drm_sched_fini is called long after all the entities > > > > > > > > are > > > > > > > > gone in > > > > > > > > drm_device cleanup. > > > > > > > > > > > > > > > > - fw scheduler hardware with preemption support. There we > > > > > > > > probably > > > > > > > > want to > > > > > > > > nuke the context by setting the tdr timeout to zero > > > > > > > > (or > > > > > > > > maybe just > > > > > > > > as > > > > > > > > long as context preemption takes to be efficient), and > > > > > > > > relying on > > > > > > > > the > > > > > > > > normal gpu reset flow to handle things. > > > > > > > > drm_sched_entity_flush > > > > > > > > kinda > > > > > > > > does this, except not really and it's a lot more > > > > > > > > focused > > > > > > > > on > > > > > > > > the > > > > > > > > ringbuffer context. So maybe we want a new > > > > > > > > drm_sched_entity_kill. > > > > > > > > > > > > > > > > For this case calling drm_sched_fini() after the 1:1 > > > > > > > > entity > > > > > > > > is gone > > > > > > > > should not find any linger jobs, it would actually be > > > > > > > > a > > > > > > > > bug > > > > > > > > somewhere if > > > > > > > > there's a job lingering. Maybe a sanity check that > > > > > > > > there's > > > > > > > > not just > > > > > > > > no > > > > > > > > jobs lingering, but also no entity left would be good > > > > > > > > here? > > > > > > > The check for lingering ones is in Christian's patch here > > > > > > > IISC. > > > > > > > At which position would you imagine the check for the > > > > > > > entity > > > > > > > being > > > > > > > performed? > > > > > > > > > > > > > > > - fw scheduler without preemption support. There we kinda > > > > > > > > need > > > > > > > > the > > > > > > > > drm_sched_flush, except blocking in fops->close is not > > > > > > > > great. So > > > > > > > > instead > > > > > > > > I think the following is better: > > > > > > > > 1. drm_sched_entity_stopped, which only stops new > > > > > > > > submissions (for > > > > > > > > paranoia) but doesn't tear down the entity > > > > > > > Who would call that function? > > > > > > > Drivers using it voluntarily could just as well stop > > > > > > > accepting > > > > > > > new jobs > > > > > > > from userspace to their entities, couldn't they? > > > > > > > > > > > > > > > 2. drm_dev_get > > > > > > > > 3. launch a worker which does a) drm_sched_flush (or > > > > > > > > drm_sched_entity_flush or whatever we call it) b) > > > > > > > > drm_sched_entity_fini > > > > > > > > + drm_sched_fini c) drm_dev_put > > > > > > > > > > > > > > > > Note that semantically this implements the refcount in > > > > > > > > the > > > > > > > > other > > > > > > > > path > > > > > > > > from Phillip: > > > > > > > > > > > > > > > > https://lore.kernel.org/all/20240903094531.29893-2-pstanner@xxxxxxxxxx/ > > > > > > > > Except it doesn't impose refcount on everyone else who > > > > > > > > doesn't need > > > > > > > > it, > > > > > > > > and it doesn't even impose refcounting on drivers that > > > > > > > > do > > > > > > > > need it > > > > > > > > because we use drm_sched_flush and a worker to achieve > > > > > > > > the > > > > > > > > same. > > > > > > > I indeed wasn't happy with the refcount approach for that > > > > > > > reason, > > > > > > > agreed. > > > > > > > > > > > > > > > Essentially helper functions for the common use-cases > > > > > > > > instead > > > > > > > > of > > > > > > > > trying to > > > > > > > > solve them all by putting drm_sched_flush as a > > > > > > > > potentially > > > > > > > > very > > > > > > > > blocking > > > > > > > > function into drm_sched_fini. > > > > > > > I'm still not able to see why it blocking would be > > > > > > > undesired – > > > > > > > as > > > > > > > far > > > > > > > as I can see, it is only invoked on driver teardown, so not > > > > > > > during > > > > > > > active operation. Teardown doesn't happen that often, and > > > > > > > it > > > > > > > can > > > > > > > (if > > > > > > > implemented correctly) only block until the driver's code > > > > > > > has > > > > > > > signaled > > > > > > > the last fences. If that doesn't happen, the block would > > > > > > > reveal > > > > > > > a > > > > > > > bug. > > > > > > > > > > > > > > But don't get me wrong: I don't want to *push* this > > > > > > > solution. I > > > > > > > just > > > > > > > want to understand when it could become a problem. > > > > > > > > > > > > > > > > > > > > > Wouldn't an explicitly blocking, separate function like > > > > > > > drm_sched_flush() or drm_sched_fini_flush() be a small, > > > > > > > doable > > > > > > > step > > > > > > > towards the right direction? > > > > > > > > > > > > I think that this is the right thing to do, yes. > > > > > > > > > > > > > > > > > > > When there are still entities with jobs the > > > > > > > > > > > > > situation > > > > > > > > > > > > > is > > > > > > > > > > > > > even > > > > > > > > > > > > > worse > > > > > > > > > > > > > since the dma_fences for those jobs can never > > > > > > > > > > > > > signal > > > > > > > > > > > > > we can > > > > > > > > > > > > > just > > > > > > > > > > > > > choose between potentially locking up core > > > > > > > > > > > > > memory > > > > > > > > > > > > > management and > > > > > > > > > > > > > random memory corruption. When drivers really > > > > > > > > > > > > > mess > > > > > > > > > > > > > it > > > > > > > > > > > > > up > > > > > > > > > > > > > that > > > > > > > > > > > > > well > > > > > > > > > > > > > let them run into a BUG_ON(). > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Christian > > > > > > > > > > > > > König<christian.koenig@xxxxxxx> > > > > > > > > > > > > > --- > > > > > > > > > > > > > drivers/gpu/drm/scheduler/sched_main.c | 19 > > > > > > > > > > > > > ++++++++++++++++++- > > > > > > > > > > > > > 1 file changed, 18 insertions(+), 1 > > > > > > > > > > > > > deletion(-) > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > > > > a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > > > > index f093616fe53c..8a46fab5cdc8 100644 > > > > > > > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > > > > > > > @@ -1333,17 +1333,34 @@ void > > > > > > > > > > > > > drm_sched_fini(struct > > > > > > > > > > > > > drm_gpu_scheduler > > > > > > > > > > > > > *sched) > > > > > > > > > > > > I agree with Sima that it should first be > > > > > > > > > > > > documented > > > > > > > > > > > > in > > > > > > > > > > > > the > > > > > > > > > > > > function's > > > > > > > > > > > > docstring what the user is expected to have done > > > > > > > > > > > > before > > > > > > > > > > > > calling the > > > > > > > > > > > > function. > > > > > > > > > > > Good point, going to update the documentation as > > > > > > > > > > > well. > > > > > > > > > > Cool thing, thx. > > > > > > > > > > Would be great if everything (not totally trivial) > > > > > > > > > > necessary to > > > > > > > > > > be done > > > > > > > > > > before _fini() is mentioned. > > > > > > > > > > > > > > > > > > > > One could also think about providing a hint at how > > > > > > > > > > the > > > > > > > > > > driver can > > > > > > > > > > do > > > > > > > > > > that. AFAICS the only way for the driver to ensure > > > > > > > > > > that > > > > > > > > > > is > > > > > > > > > > to > > > > > > > > > > maintain > > > > > > > > > > its own, separate list of submitted jobs. > > > > > > > > > Even with a duplicated pending list it's actually > > > > > > > > > currently > > > > > > > > > impossible to do > > > > > > > > > this fully cleanly. > > > > > > > > > > > > > > > > > > The problem is that the dma_fence object gives no > > > > > > > > > guarantee > > > > > > > > > when > > > > > > > > > callbacks > > > > > > > > > are processed, e.g. they can be both processed from > > > > > > > > > interrupt > > > > > > > > > context as > > > > > > > > > well as from a CPU which called > > > > > > > > > dma_fence_is_signaled(). > > > > > > > > > > > > > > > > > > So when a driver (or drm_sched_fini) waits for the last > > > > > > > > > submitted > > > > > > > > > fence it > > > > > > > > > actually can be that the drm_sched object still needs > > > > > > > > > to do > > > > > > > > > some > > > > > > > > > processing. > > > > > > > > > See the hack in amdgpu_vm_tlb_seq() for more background > > > > > > > > > on > > > > > > > > > the > > > > > > > > > problem. > > > > > > > Oh dear ^^' > > > > > > > We better work towards fixing that centrally > > > > > > > > > > > > > > Thanks, > > > > > > > P. > > > > > > > > > > > > > > > > > > > > > > So I thought this should be fairly easy because of the > > > > > > > > sched > > > > > > > > hw/public > > > > > > > > fence split: If we wait for both all jobs to finish and > > > > > > > > for > > > > > > > > all > > > > > > > > the > > > > > > > > sched > > > > > > > > work/tdr work to finish, and we make sure there's no > > > > > > > > entity > > > > > > > > existing > > > > > > > > that's not yet stopped we should catch them all? > > > > > > > > > > > > Unfortunately not. > > > > > > > > > > > > Even when you do a dma_fence_wait() on the last submission it > > > > > > can > > > > > > still be > > > > > > that another CPU is executing the callbacks to wake up the > > > > > > scheduler work > > > > > > item and cleanup the job. > > > > > > > > > > > > That's one of the reasons why I think the design of keeping > > > > > > the > > > > > > job > > > > > > alive is > > > > > > so extremely awkward. The dma_fence as representation of the > > > > > > hw > > > > > > submission > > > > > > has a much better defined state machine and lifetime. > > > > > > > > > > > > Regards, > > > > > > Christian. > > > > > > > > > > > > > > Or at least I think > > > > > > > > it's > > > > > > > > a bug if any other code even tries to touch the hw fence. > > > > > > > > > > > > > > > > If you have any other driver code which relies on the rcu > > > > > > > > freeing > > > > > > > > then I > > > > > > > > think that's just a separate concern and we can ignore > > > > > > > > that > > > > > > > > here > > > > > > > > since the > > > > > > > > fences themselves will till get rcu-delay freed even if > > > > > > > > drm_sched_fini has > > > > > > > > finished. > > > > > > > > -Sima > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > Christian. > > > > > > > > > > > > > > > > > > > P. > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Christian. > > > > > > > > > > > > > > > > > > > > > > > P. > > > > > > > > > > > > > > > > > > > > > > > > > drm_sched_wqueue_stop(sched); > > > > > > > > > > > > > + /* > > > > > > > > > > > > > + * Tearing down the scheduler wile there are > > > > > > > > > > > > > still > > > > > > > > > > > > > unprocessed jobs can > > > > > > > > > > > > > + * lead to use after free issues in the > > > > > > > > > > > > > scheduler > > > > > > > > > > > > > fence. > > > > > > > > > > > > > + */ > > > > > > > > > > > > > + WARN_ON(!list_empty(&sched->pending_list)); > > > > > > > > > > > > > + > > > > > > > > > > > > > for (i = DRM_SCHED_PRIORITY_KERNEL; i < > > > > > > > > > > > > > sched- > > > > > > > > > > > > > > num_rqs; > > > > > > > > > > > > > i++) > > > > > > > > > > > > > { > > > > > > > > > > > > > struct drm_sched_rq *rq = sched- > > > > > > > > > > > > > > sched_rq[i]; > > > > > > > > > > > > > spin_lock(&rq->lock); > > > > > > > > > > > > > - list_for_each_entry(s_entity, &rq- > > > > > > > > > > > > > > entities, > > > > > > > > > > > > > list) > > > > > > > > > > > > > + list_for_each_entry(s_entity, &rq- > > > > > > > > > > > > > > entities, > > > > > > > > > > > > > list) { > > > > > > > > > > > > > + /* > > > > > > > > > > > > > + * The justification for this > > > > > > > > > > > > > BUG_ON() > > > > > > > > > > > > > is > > > > > > > > > > > > > that tearing > > > > > > > > > > > > > + * down the scheduler while jobs > > > > > > > > > > > > > are > > > > > > > > > > > > > pending > > > > > > > > > > > > > leaves > > > > > > > > > > > > > + * dma_fences unsignaled. Since we > > > > > > > > > > > > > have > > > > > > > > > > > > > dependencies > > > > > > > > > > > > > + * from the core memory management > > > > > > > > > > > > > to > > > > > > > > > > > > > eventually signal > > > > > > > > > > > > > + * dma_fences this can trivially > > > > > > > > > > > > > lead to > > > > > > > > > > > > > a > > > > > > > > > > > > > system wide > > > > > > > > > > > > > + * stop because of a locked up > > > > > > > > > > > > > memory > > > > > > > > > > > > > management. > > > > > > > > > > > > > + */ > > > > > > > > > > > > > + BUG_ON(spsc_queue_count(&s_entity- > > > > > > > > > > > > > > job_queue)); > > > > > > > > > > > > > + > > > > > > > > > > > > > /* > > > > > > > > > > > > > * Prevents reinsertion and marks > > > > > > > > > > > > > job_queue > > > > > > > > > > > > > as idle, > > > > > > > > > > > > > * it will removed from rq in > > > > > > > > > > > > > drm_sched_entity_fini > > > > > > > > > > > > > * eventually > > > > > > > > > > > > > */ > > > > > > > > > > > > > s_entity->stopped = true; > > > > > > > > > > > > > + } > > > > > > > > > > > > > spin_unlock(&rq->lock); > > > > > > > > > > > > > kfree(sched->sched_rq[i]); > > > > > > > > > > > > > } > > > > > > > > > > > > > > >
