On Mon, Nov 11, 2024 at 04:54:57PM +0100, Christian König wrote: > Am 11.11.24 um 15:00 schrieb Joonas Lahtinen: > > Quoting Christian König (2024-11-11 13:34:12) > > > Am 11.11.24 um 11:10 schrieb Simona Vetter: > > > > On Mon, Nov 11, 2024 at 10:00:17AM +0200, Joonas Lahtinen wrote: > > > > > Back from some time off and will try to answer below. > > > > > > > > > > Adding Dave and Sima as this topic has been previously discussed to some > > > > > extent and will be good to reach common understanding about what the > > > > > series is trying to do and what is the difference to the AMD debugging > > > > > model. > > > > I chatted about this thread a bit on irc with folks, and I think an > > > > orthogonal issue is the question, what should be in ttm-utils? I've asked > > > > Matt to type up a DOC patch once we have some consensus, since imo the > > > > somewhat lackluster documentation situation for ttm is also somewhat a > > > > cause for these big threads on various different topics. Aside from the > > > > fact that gpu memory management is just hard. > > > > > > > > On the uapi/design aspect, I think this would serve well with a patch to > > > > drm-uapi.rst that adds a debugging section? At least once we have some > > > > rough consensus across drivers, and more importantly userspace in the form > > > > of gdb upstream (at least I'm not aware of any other upstream debugger > > > > patches, I think amd's rocm stuff is also gdb-only). > > > Yeah that seems to be a really good idea. Similar design ideas came up > > > AMD internally as well but where dropped after pointing people to > > > pidfd_getfd(). > > > > > > But the bigger problem seems to be that the design doesn't seems to take > > > the dma_fence requirements into account. > > Where would you deduce that? > > XE is based on a preemption fence based memory management. > > > We specifically limit the debugging to Long Running contexts which don't > > depend on dma_fences. > > That doesn't matter. > > As long as you don't have a page fault (HMM) based memory management you > still have that inter dependency and at least the public available XE code > doesn't seem to have that. > > > > In other words attaching gdb to a pid seems to stop the GPU thread of > > > this pid without waiting for the XE preemption nor end of operation fence. > > > > > > I mean if the GPU threads are preempted that could work, but yeah not > > > like this :) > > For us, hitting a breakpoint inside the workload would always violate > > any dma_fence timeout for the submitted workload, as the HW context can't > > be switched out while in the breakpoint. > > That is clearly *not* something you can do without changing your memory > management. > > > For any dma_fence workload the guarantee is that that it completes > > within reasonable time after submission (guaranteed by the submitter). I > > don't see how you could really allow interactive debugging of a > > breakpoint under those restrictions anyway even if pre-emption was > > supported as the workload would not finish in <10 seconds? > > Yeah that is the whole point, this is impossible as far as we know. > > > For i915 we did have the "pre-emptable but indefinitely long dma_fence workloads" > > concept at one point and that was rejected after the lengthy discussion. > > I mean I tried that more than once myself and we have multiple requests for > this on the AMD side from customers. So far nobody came up with a solution > which actually works correctly. > > > So I think only way to allow interactive debugging is to avoid the > > dma_fences. Curious to hear if there are ideas for otherwise. > > You need to guarantee somehow that the process is taken from the hardware so > that the preemption fence can signal. > Our preemption fences have this functionality. A preemption fence issues a suspend execution command to the firmware. The firmware, in turn, attempts to preempt the workload. If it doesn't respond within a specified period, it resets the hardware queue, sends a message to KMD, bans the software queue, and signals the preemption fence. We provide even more protection than that. If, for some reason, the firmware doesn't respond within a longer timeout period, the KMD performs a device reset, ban the offending software queue(s), and will signal the preemption fences. This flow remains the same whether a debugger is attached or, for example, a user submits a 10-minute non-preemptable workload. In either case, other processes are guaranteed to make forward progress. The example above illustrates the memory oversubscription case, where two processes are using 51% of the memory. Another preemption scenario involves two processes sharing hardware resources. Our firmware follows the same flow here. If an LR workload is using a hardware resource and a DMA-fence workload is waiting, and if the LR workload doesn't preempt the in a timely manner, the firmware issues a hardware reset, notifies KMD, and bans the LR software queue. The DMA-fence workload then can make forward progress With the above in mind, this is why I say that if a user tries to run a game and a non-preemptable LR workload, either oversubscribing memory or sharing hardware resources, it is unlikely to work well. However, I don't think this is a common use case. I would expect that when a debugger is open, it is typically by a power user who knows how to disable other GPU tasks (e.g., by enabling software rendering or using a machine without any display). Given this, please to reconsider your position. > This means that a breakpoint or core dump doesn't halt GPU threads, but > rather suspends them. E.g. all running wave data is collected into a state > bag which can be restored later on. > > I was under the impression that those long running compute threads do > exactly that, but when the hardware can't switch out the GPU thread/process > while in a break then that isn't the case. > > As long as you don't find a way to avoid that this patch set is a pretty > clear NAK from my side as DMA-buf and TTM maintainer. > I believe this is addressed above. Matt > What might work is to keep the submission on the hardware in the break state > but forbid any memory access. This way you can signal your preemption fence > even when the hardware isn't made available. > > Before you continue XE setups a new pre-emption fence and makes sure that > all page tables etc... are up to date. > > Could be tricky to get this right if completion fence based submissions are > mixed in as well, but that gives you at least a direction you could > potentially go. > > Regards, > Christian. > > > > > Regards, Joonas > > > > > Regards, > > > Christian. > > > > > > > Some wash-up thoughts from me below, but consider them fairly irrelevant > > > > since I think the main driver for these big questions here should be > > > > gdb/userspace. > > > > > > > > > Quoting Christian König (2024-11-07 11:44:33) > > > > > > Am 06.11.24 um 18:00 schrieb Matthew Brost: > > > > > > > > > > > > [SNIP] > > > > > > > > > > > > This is not a generic interface that anyone can freely access. The same > > > > > > permissions used by ptrace are checked when opening such an interface. > > > > > > See [1] [2]. > > > > > > > > > > > > [1]https://patchwork.freedesktop.org/patch/617470/?series=136572&rev=2 > > > > > > [2]https://patchwork.freedesktop.org/patch/617471/?series=136572&rev=2 > > > > > > > > > > > > > > > > > > Thanks a lot for those pointers, that is exactly what I was looking for. > > > > > > > > > > > > And yeah, it is what I feared. You are re-implementing existing functionality, > > > > > > but see below. > > > > > Could you elaborate on what this "existing functionality" exactly is? > > > > > I do not think this functionality exists at this time. > > > > > > > > > > The EU debugging architecture for Xe specifically avoids the need for GDB > > > > > to attach with ptrace to the CPU process or interfere with the CPU process for > > > > > the debugging via parasitic threads or so. > > > > > > > > > > Debugger connection is opened to the DRM driver for given PID (which uses the > > > > > ptrace may access check for now) after which the all DRM client of that > > > > > PID are exposed to the debugger process. > > > > > > > > > > What we want to expose via that debugger connection is the ability for GDB to > > > > > read/write the different GPU VM address spaces (ppGTT for Intel GPUs) just like > > > > > the EU threads would see them. Note that the layout of the ppGTT is > > > > > completely up to the userspace driver to setup and is mostly only partially > > > > > equal to the CPU address space. > > > > > > > > > > Specifically as part of reading/writing the ppGTT for debugging purposes, > > > > > there are deep flushes needed: for example flushing instruction cache > > > > > when adding/removing breakpoints. > > > > > > > > > > Maybe that will explain the background. I elaborate on this at the end some more. > > > > > > > > > > > kmap/vmap are used everywhere in the DRM subsystem to access BOs, so I’m > > > > > > failing to see the problem with adding a simple helper based on existing > > > > > > code. > > > > > > > > > > > > What#s possible and often done is to do kmap/vmap if you need to implement a > > > > > > CPU copy for scanout for example or for copying/validating command buffers. > > > > > > But that usually requires accessing the whole BO and has separate security > > > > > > checks. > > > > > > > > > > > > When you want to access only a few bytes of a BO that sounds massively like > > > > > > a peek/poke like interface and we have already rejected that more than once. > > > > > > There even used to be standardized GEM IOCTLs for that which have been > > > > > > removed by now. > > > > > Referring to the explanation at top: These IOCTL are not for the debugging target > > > > > process to issue. The peek/poke interface is specifically for GDB only > > > > > to facilitate the emulation of memory reads/writes on the GPU address > > > > > space as they were done by EUs themselves. And to recap: for modifying > > > > > instructions for example (add/remove breakpoint), extra level of cache flushing is > > > > > needed which is not available to regular userspace. > > > > > > > > > > I specifically discussed with Sima on the difference before moving forward with this > > > > > design originally. If something has changed since then, I'm of course happy to rediscuss. > > > > > > > > > > However, if this code can't be added, not sure how we would ever be able > > > > > to implement core dumps for GPU threads/memory? > > > > > > > > > > > If you need to access BOs which are placed in not CPU accessible memory then > > > > > > implement the access callback for ptrace, see amdgpu_ttm_access_memory for > > > > > > an example how to do this. > > > > > As also mentioned above, we don't work via ptrace at all when it comes > > > > > to debugging the EUs. The only thing used for now is the ptrace_may_access to > > > > > implement similar access restrictions as ptrace has. This can be changed > > > > > to something else if needed. > > > > > > > > > > > Ptrace access via vm_operations_struct.access → ttm_bo_vm_access. > > > > > > > > > > > > This series renames ttm_bo_vm_access to ttm_bo_access, with no code changes. > > > > > > > > > > > > The above function accesses a BO via kmap if it is in SYSTEM / TT, > > > > > > which is existing code. > > > > > > > > > > > > This function is only exposed to user space via ptrace permissions. > > > > > Maybe this sentence is what caused the confusion. > > > > > > > > > > Userspace is never exposed with peek/poke interface, only the debugger > > > > > connection which is its own FD. > > > > > > > > > > > In this series, we implement a function [3] similar to > > > > > > amdgpu_ttm_access_memory for the TTM vfunc access_memory. What is > > > > > > missing is non-visible CPU memory access, similar to > > > > > > amdgpu_ttm_access_memory_sdma. This will be addressed in a follow-up and > > > > > > was omitted in this series given its complexity. > > > > > > > > > > > > So, this looks more or less identical to AMD's ptrace implementation, > > > > > > but in GPU address space. Again, I fail to see what the problem is here. > > > > > > What am I missing? > > > > > > > > > > > > > > > > > > The main question is why can't you use the existing interfaces directly? > > > > > We're not working on the CPU address space or BOs. We're working > > > > > strictly on the GPU address space as would be seen by an EU thread if it > > > > > accessed address X. > > > > > > > > > > > Additional to the peek/poke interface of ptrace Linux has the pidfd_getfd > > > > > > system call, see herehttps://man7.org/linux/man-pages/man2/pidfd_getfd.2.html. > > > > > > > > > > > > The pidfd_getfd() allows to dup() the render node file descriptor into your gdb > > > > > > process. That in turn gives you all the access you need from gdb, including > > > > > > mapping BOs and command submission on behalf of the application. > > > > > We're not operating on the CPU address space nor are we operating on BOs > > > > > (there is no concept of BO in the EU debug interface). Each VMA in the VM > > > > > could come from anywhere, only the start address and size matter. And > > > > > neither do we need to interfere with the command submission of the > > > > > process under debug. > > > > > > > > > > > As far as I can see that allows for the same functionality as the eudebug > > > > > > interface, just without any driver specific code messing with ptrace > > > > > > permissions and peek/poke interfaces. > > > > > > > > > > > > So the question is still why do you need the whole eudebug interface in the > > > > > > first place? I might be missing something, but that seems to be superfluous > > > > > > from a high level view. > > > > > Recapping from above. It is to allow the debugging of EU threads per DRM > > > > > client, completely independent of the CPU process. If ptrace_may_acces > > > > > is the sore point, we could consider other permission checks, too. There > > > > > is no other connection to ptrace in this architecture as single > > > > > permission check to know if PID is fair game to access by debugger > > > > > process. > > > > > > > > > > Why no parasitic thread or ptrace: Going forward, binding the EU debugging to > > > > > the DRM client would also pave way for being able to extend core kernel generated > > > > > core dump with each DRM client's EU thread/memory dump. We have similar > > > > > feature called "Offline core dump" enabled in the downstream public > > > > > trees for i915, where we currently attach the EU thread dump to i915 error state > > > > > and then later combine i915 error state with CPU core dump file with a > > > > > tool. > > > > > > > > > > This is relatively little amount of extra code, as this baseline series > > > > > already introduces GDB the ability to perform the necessary actions. > > > > > It's just the matter of kernel driver calling: "stop all threads", then > > > > > copying the memory map and memory contents for GPU threads, just like is > > > > > done for CPU threads. > > > > > > > > > > With parasitic thread injection, not sure if there is such way forward, > > > > > as it would seem to require to inject quite abit more logic to core kernel? > > > > > > > > > > > It's true that the AMD KFD part has still similar functionality, but that is > > > > > > because of the broken KFD design of tying driver state to the CPU process > > > > > > (which makes it inaccessible for gdb even with imported render node fd). > > > > > > > > > > > > Both Sima and I (and partially Dave as well) have pushed back on the KFD > > > > > > approach. And the long term plan is to get rid of such device driver specific > > > > > > interface which re-implement existing functionality just differently. > > > > > Recapping, this series is not adding it back. The debugger connection > > > > > is a separate FD from the DRM one, with separate IOCTL set. We don't allow > > > > > the DRM FD any new operations based on ptrace is attached or not. We > > > > > don't ever do that check even. > > > > > > > > > > We only restrict the opening of the debugger connection to given PID with > > > > > ptrace_may_access check for now. That can be changed to something else, > > > > > if necessary. > > > > Yeah I think unnecessarily tying gpu processes to cpu processes is a bad > > > > thing, least because even today all the svm discussions we have still hit > > > > clear use-cases, where a 1:1 match is not wanted (like multiple gpu svm > > > > sections with offsets). Not even speaking of all the gpu usecases where > > > > the gpu vm space is still entirely independent of the cpu side. > > > > > > > > So that's why I think this entirely separate approach looks like the right > > > > one, with ptrace_may_access as the access control check to make sure we > > > > match ptrace on the cpu side. > > > > > > > > But there's very obviously a bikeshed to be had on what the actual uapi > > > > should look like, especially how gdb opens up a gpu debug access fd. But I > > > > also think that's not much on drm to decide, but whatever gdb wants. And > > > > then we aim for some consistency on that lookup/access control part > > > > (ideally, I might be missing some reasons why this is a bad idea) across > > > > drm drivers. > > > > > > > > > > So you need to have a really really good explanation why the eudebug interface > > > > > > is actually necessary. > > > > > TL;DR The main point is to decouple the debugging of the EU workloads from the > > > > > debugging of the CPU process. This avoids the interference with the CPU process with > > > > > parasitic thread injection. Further this also allows generating a core dump > > > > > without any GDB connected. There are also many other smaller pros/cons > > > > > which can be discussed but for the context of this patch, this is the > > > > > main one. > > > > > > > > > > So unlike parasitic thread injection, we don't unlock any special IOCTL for > > > > > the process under debug to be performed by the parasitic thread, but we > > > > > allow the minimal set of operations to be performed by GDB as if those were > > > > > done on the EUs themselves. > > > > > > > > > > One can think of it like the minimal subset of ptrace but for EU threads, > > > > > not the CPU threads. And thus, building on this it's possible to extend > > > > > the core kernel generated core dumps with DRM specific extension which > > > > > would contain the EU thread/memory dump. > > > > It might be good to document (in that debugging doc patch probably) why > > > > thread injection is not a great option, and why the tradeoffs for > > > > debugging are different than for for checkpoint/restore, where with CRIU > > > > we landed on doing most of this in userspace, and often requiring > > > > injection threads to make it all work. > > > > > > > > Cheers, Sima > > > > > > > > > Regards, Joonas > > > > > > > > > > > Regards, > > > > > > Christian. > > > > > > > > > > > > > > > > > > > > > > > > Matt > > > > > > > > > > > > [3]https://patchwork.freedesktop.org/patch/622520/?series=140200&rev=6 > > > > > > > > > > > > > > > > > > Regards, > > > > > > Christian. > > > > > > > > > > > > > > > > > > Matt > > > > > > > > > > > > > > > > > > Regards, > > > > > > Christian. > > > > > >
