On Tue, 2024-11-12 at 15:41 +0200, Joonas Lahtinen wrote: > (+ Thomas) > > Quoting Christian König (2024-11-12 11:23:36) > > Am 11.11.24 um 23:45 schrieb Matthew Brost: > > > > [SNIP] > > > > 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. > > > > > > Yeah that is pretty much the same argumentation I have heard before > > and it > > turned out to not be working. > > > > > > The example above illustrates the memory oversubscription case, > > where two > > processes are using 51% of the memory. > > > > > > That isn't even necessary. We have seen applications dying just > > because the > > core memory management tried to join back small pages into huge > > pages in an > > userptr. > > > > That the core memory management jumps in and requests that the pre- > > emption > > fence signals can happen all the time. > > Ouch. Does there happen to be a known reproducer for this behavior or > maybe > bug report? > > > You can mitigate that a bit, Fedora for example disables joining > > back small > > pages into huge pages by default for example and we even had people > > suggesting > > to use mprotect() so that userptrs VMAs don't fork() any more > > (which is of > > course completely illegal). > > > > But my long term take away is that you can't block all causes of > > sudden > > requests to let a pre-emption fence signal. > > I think this problem equally applies to the LR-workloads like the EU > debugging ones. > > > 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. > > > > > > The key point here is that this isn't stable, you can do that as a > > tech demo > > but it can always be that debugging an application just randomly > > dies. And > > believe me AMD has tried this to a rather extreme extend as well. > > It's not really only limited to the debuggable applications at all, > the > normal LR workloads are equally impacted as far as I understand. Just > harder to catch the issue with LR-workloads if the pre-emption fence > signaling is sporadic. > > > What you could potentially work is to taint the kernel and make > > sure that this > > function is only available to user who absolutely know what they > > are doing. > > > > But I would say we can only allow that if all other options have > > been exercised > > and doing it like this is really the only option left. > > It sounds like servicing the memory pre-empt fence by stealing the > pages from underneath the workload would be the way to resolve this > issue. > > This has been extensively discussed already, but was expected to > really > only be needed for low-on-memory scenarios. However it now seems like > the need is much earlier due to the random userptr page joining by > core > mm. Just to clarify here: In Long-Running mode with recoverable pagefaults enabled we don't have any preempt-fences, but rather just zap the PTEs pointing to the affected memory and flush TLB. So from a memory resource POW a breakpoint should be safe, and no mmu notifier nor shrinker will be blocked. Nor will there be any jobs with published dma-fences depending on the job blocked either temporarily by a pagefault or long-term by a debugger breakpoint. /Thomas > > If that is done and the memory pre-empt fence is serviced even for > debuggable contexts, do you have further concerns with the presented > approach > from dma-buf and drm/sched perspective? > > Regards, Joonas > > > > > Regards, > > Christian. > > > > > > 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&re > > v=2 > > > > [2]https://patchwork.freedesktop.org/patch/617471/?series=136572&re > > v=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&re > > v=6 > > > > > > Regards, > > Christian. > > > > > > Matt > > > > > > Regards, > > Christian. > > > > > >
