Re: [Regression] drm/scheduler: track GPU active time per entity

[Daniel Vetter]

On Thu, Apr 06, 2023 at 05:33:18PM +0200, Christian König wrote:
> Am 06.04.23 um 17:24 schrieb Lucas Stach:
> > Am Donnerstag, dem 06.04.2023 um 16:21 +0200 schrieb Christian König:
> > > Am 06.04.23 um 12:45 schrieb Lucas Stach:
> > > > Am Donnerstag, dem 06.04.2023 um 10:27 +0200 schrieb Daniel Vetter:
> > > > > On Thu, 6 Apr 2023 at 10:22, Christian König <christian.koenig@xxxxxxx> wrote:
> > > > > > Am 05.04.23 um 18:09 schrieb Luben Tuikov:
> > > > > > > On 2023-04-05 10:05, Danilo Krummrich wrote:
> > > > > > > > On 4/4/23 06:31, Luben Tuikov wrote:
> > > > > > > > > On 2023-03-28 04:54, Lucas Stach wrote:
> > > > > > > > > > Hi Danilo,
> > > > > > > > > > 
> > > > > > > > > > Am Dienstag, dem 28.03.2023 um 02:57 +0200 schrieb Danilo Krummrich:
> > > > > > > > > > > Hi all,
> > > > > > > > > > > 
> > > > > > > > > > > Commit df622729ddbf ("drm/scheduler: track GPU active time per entity")
> > > > > > > > > > > tries to track the accumulated time that a job was active on the GPU
> > > > > > > > > > > writing it to the entity through which the job was deployed to the
> > > > > > > > > > > scheduler originally. This is done within drm_sched_get_cleanup_job()
> > > > > > > > > > > which fetches a job from the schedulers pending_list.
> > > > > > > > > > > 
> > > > > > > > > > > Doing this can result in a race condition where the entity is already
> > > > > > > > > > > freed, but the entity's newly added elapsed_ns field is still accessed
> > > > > > > > > > > once the job is fetched from the pending_list.
> > > > > > > > > > > 
> > > > > > > > > > > After drm_sched_entity_destroy() being called it should be safe to free
> > > > > > > > > > > the structure that embeds the entity. However, a job originally handed
> > > > > > > > > > > over to the scheduler by this entity might still reside in the
> > > > > > > > > > > schedulers pending_list for cleanup after drm_sched_entity_destroy()
> > > > > > > > > > > already being called and the entity being freed. Hence, we can run into
> > > > > > > > > > > a UAF.
> > > > > > > > > > > 
> > > > > > > > > > Sorry about that, I clearly didn't properly consider this case.
> > > > > > > > > > 
> > > > > > > > > > > In my case it happened that a job, as explained above, was just picked
> > > > > > > > > > > from the schedulers pending_list after the entity was freed due to the
> > > > > > > > > > > client application exiting. Meanwhile this freed up memory was already
> > > > > > > > > > > allocated for a subsequent client applications job structure again.
> > > > > > > > > > > Hence, the new jobs memory got corrupted. Luckily, I was able to
> > > > > > > > > > > reproduce the same corruption over and over again by just using
> > > > > > > > > > > deqp-runner to run a specific set of VK test cases in parallel.
> > > > > > > > > > > 
> > > > > > > > > > > Fixing this issue doesn't seem to be very straightforward though (unless
> > > > > > > > > > > I miss something), which is why I'm writing this mail instead of sending
> > > > > > > > > > > a fix directly.
> > > > > > > > > > > 
> > > > > > > > > > > Spontaneously, I see three options to fix it:
> > > > > > > > > > > 
> > > > > > > > > > > 1. Rather than embedding the entity into driver specific structures
> > > > > > > > > > > (e.g. tied to file_priv) we could allocate the entity separately and
> > > > > > > > > > > reference count it, such that it's only freed up once all jobs that were
> > > > > > > > > > > deployed through this entity are fetched from the schedulers pending list.
> > > > > > > > > > > 
> > > > > > > > > > My vote is on this or something in similar vain for the long term. I
> > > > > > > > > > have some hope to be able to add a GPU scheduling algorithm with a bit
> > > > > > > > > > more fairness than the current one sometime in the future, which
> > > > > > > > > > requires execution time tracking on the entities.
> > > > > > > > > Danilo,
> > > > > > > > > 
> > > > > > > > > Using kref is preferable, i.e. option 1 above.
> > > > > > > > I think the only real motivation for doing that would be for generically
> > > > > > > > tracking job statistics within the entity a job was deployed through. If
> > > > > > > > we all agree on tracking job statistics this way I am happy to prepare a
> > > > > > > > patch for this option and drop this one:
> > > > > > > > https://lore.kernel.org/all/20230331000622.4156-1-dakr@xxxxxxxxxx/T/#u
> > > > > > > Hmm, I never thought about "job statistics" when I preferred using kref above.
> > > > > > > The reason kref is attractive is because one doesn't need to worry about
> > > > > > > it--when the last user drops the kref, the release is called to do
> > > > > > > housekeeping. If this never happens, we know that we have a bug to debug.
> > > > > > Yeah, reference counting unfortunately have some traps as well. For
> > > > > > example rarely dropping the last reference from interrupt context or
> > > > > > with some unexpected locks help when the cleanup function doesn't expect
> > > > > > that is a good recipe for problems as well.
> > > > > > 
> > > > Fully agreed.
> > > > 
> > > > > > > Regarding the patch above--I did look around the code, and it seems safe,
> > > > > > > as per your analysis, I didn't see any reference to entity after job submission,
> > > > > > > but I'll comment on that thread as well for the record.
> > > > > > Reference counting the entities was suggested before. The intentionally
> > > > > > avoided that so far because the entity might be the tip of the iceberg
> > > > > > of stuff you need to keep around.
> > > > > > 
> > > > > > For example for command submission you also need the VM and when you
> > > > > > keep the VM alive you also need to keep the file private alive....
> > > > > Yeah refcounting looks often like the easy way out to avoid
> > > > > use-after-free issue, until you realize you've just made lifetimes
> > > > > unbounded and have some enourmous leaks: entity keeps vm alive, vm
> > > > > keeps all the bo alives, somehow every crash wastes more memory
> > > > > because vk_device_lost means userspace allocates new stuff for
> > > > > everything.
> > > > > 
> > > > > If possible a lifetime design where lifetimes have hard bounds and you
> > > > > just borrow a reference under a lock (or some other ownership rule) is
> > > > > generally much more solid. But also much harder to design correctly
> > > > > :-/
> > > > > 
> > > > The use we are discussing here is to keep the entity alive as long as
> > > > jobs from that entity are still active on the HW. While there are no
> > > > hard bounds on when a job will get inactive, at least it's not
> > > > unbounded. On a crash/fault the job will be removed from the hardware
> > > > pretty soon.
> > > > 
> > > > Well behaved jobs after a application shutdown might take a little
> > > > longer, but I don't really see the new problem with keeping the entity
> > > > alive? As long as a job is active on the hardware, we can't throw out
> > > > the VM or BOs, no difference whether the entity is kept alive or not.
> > > Exactly that's the problem. VM & BOs are dropped as soon as the process
> > > is destroyed, we *don't* wait for the hw to finish before doing so.
> > > 
> > > Just the backing store managed by all the house keeping objects isn't
> > > freed until the hw is idle preventing a crash or accessing freed memory.
> > > 
> > > This behavior is rather important for the OOM killer since we need to be
> > > able to tear down the process as fast as possible in that case.
> > > 
> > Are you talking about the dropping of pending jobs in
> > drm_sched_entity_kill? I'm certainly not trying to change that in any
> > way. Those aren't put onto the hardware yet, so we can always safely
> > drop them and do so as fast as possible.
> > 
> > What I'm concerned about are the jobs that are already scheduled on the
> > HW. At least with Vivante hardware there is no race free way to get rid
> > of jobs once they are put on the ring. So whatever the scheduler or DRM
> > core is doing, we have to hold on to the BOs and GPU memory management
> > structures to keep the hardware from operating on freed memory.
> > 
> > That's already a lot of memory, so I don't really see the issue with
> > keeping the entity around in a quiescent state until all the currently
> > queued jobs have left the HW.
> > 
> > > Changing that is possible, but that's quite a huge change I'm not really
> > > willing to do just for tracking the time spend.
> > > 
> > Yea, it's a big change and whether it's a good idea really depends on
> > what we a gaining from it. You seem to see quite low value in "just
> > tracking the time spent" and that might be true, but it also forces all
> > drivers that want to implement fdinfo to roll their own time tracking.
> > I would rather see more of this moved to the scheduler and thus shared
> > between drivers.
> 
> That's generally a good idea, but if that means that we need to restructure
> the whole entity handling then I would object. That's simply not worth it
> when we can implement it differently.
> 
> What we could do is to keep the submitted fences around in the entity.
> Similar to the tracking amdgpu does, see struct amdgpu_ctx_entity.
> 
> This way the entity doesn't needs to stay around after it delivered the job
> to the hw.

I've done a _very_ cursory look, but from that the design seems to be that
we only keep something very small around to avoid oopsing (just
drm_sched_entity) and the overall gpu ctx goes away synchronously (more or
less) when userspace destroys it.

And then the actual fdinfo reporting or any reporting is a pure pull model
where you come from drm_file -> gpu ctx -> amdgpu_ctx_entity and have
borrowed references by holding enough locks.

The push model, where the scheduler job actively pushes the stats all the
way to the userspace/uapi ctx object is imo impossible to get right.
i915-gem tried a few times, and solutions start with sprinkling rcu
everywhere and only get worse. Imo absolute no-go.

If I understand this right then yes I think extracting the
amgpu_ctx_entity split into a bit of helper. Might want to extract the
context xarray and lookup too and put it into drm_file, including the
amdgpu_ctx_mgr->lock and iterating over stats for fdinfo maybe too to make
this really worth it.

Of all the failed attempts I've seen for exposing ctx stats this (amdgpu
push model) is imo the only solid one.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch