On 10/21/2023 10:00 AM, Greg KH wrote:> This is now outside of my subsystems to review, sorry. It's going to> have to go through the x86 tree, so you are going to have to convince > them that this is something that actually matters and is needed by > people, as maintaining it over time is going to add to their workload. Tracing is not needed, per se, but it can be incredibly useful for debugging certain types of problems. I think the utility of tracing is well accepted by the community. Quoting Peter Zijlstra: "...tracepoints in general are useful" Quoting Andy Shevchenko: "...you may trace any IO on some architectures (at least x86), it's called mmiotracer (I have used it like 5 years ago or so to trace UART)" Similar trace functionality is already present in the kernel today (CONFIG_MMIOTRACE and CONFIG_TRACE_MMIO_ACCESS) and it does anecdotally get used. However, as mentioned in the patch description, those tracing features don't work with port-based I/O hence the need for this patch. I don't see how this is going to create a maintenance burden. It adds two lines of code to a macro that rarely if ever changes. >> --- a/arch/x86/boot/Makefile >> +++ b/arch/x86/boot/Makefile > > Note, you are keeping tracing from working in a few areas that might not > be good... >> Now I know why you did that for this patch (i.e. so early boot doesn't > get the printk mess), but that kind of defeats the use of tracepoints at > all for this part of the kernel, is that ok? Are any existing > tracepoints now removed? Some of the kernel sources (arch/x86/boot/* and arch/x86/realmode/*) are not part of the kernel proper and they don't have the infrastructure to support tracepoints. When these sources include header files that reference tracepoints it causes compiler errors. I previously worked around this issue with include guard checks but you objected to that: "I see what you are doing here in trying to see if a .h file has been included already, but now you are making an assumption on both the .h file ordering, and the #ifdef guard for those .h files, which are something that we almost never remember or even consider when dealing with .h files files." Therefore I implemented a more reliable mechanism to disable tracepoints. I explained this earlier in the thread: "What we need is to disable tracepoints altogether in arch/x86/boot/* so I added -DDISABLE_TRACEPOINTS to the relevant Makefiles and I added a check for that symbol in tracepoint-defs.h. I will submit a v4 version of my patch with these changes shortly. This resolves the problem with <asm/msr.h> as well. After applying the v4 patch I was able to call rdmsr()/wrmsr() from arch/x86/boot/misc.c. Theoretically we can now remove arch/x86/boot/msr.h but I had trouble with that due to compiler warnings and errors. The include files in arch/x86/boot are a mess. Maybe this can be cleaned up in another patch." >> --- a/arch/x86/include/asm/cmpxchg_32.h >> +++ b/arch/x86/include/asm/cmpxchg_32.h > > Why are these needed to be changed at all? What code changes with it, > and it's not mentioned in the changelog, so why is it required? I did mention these changes in the changelog: "fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()" These warnings appear to be a latent defect which was triggered by the include file changes in this patch. I assume that introducing compiler warnings (even indirectly) is not allowed so I fixed them on this patch. >> + if (tracepoint_enabled(portio_write)) \ >> + do_trace_portio_write(value, port, #bwl[0]); \ > > Your level of indirection here seems deep, why doesn't > do_trace_portio_write() belong in a .h file here and do the > tracepoint_enabled() check? > > Is this a by-product of the tracing macros that require this deeper > callchain to happen? Please reference Documentation/trace/tracepoints.rst: "If you require calling a tracepoint from a header file, it is not recommended to call one directly or to use the trace_<tracepoint>_enabled() function call, as tracepoints in header files can have side effects..." The tracepoint_enabled() macro is very efficient and causes only one instruction of overhead (a nop) when tracing is disabled. I verified this by disassembling vmlinux. >> +obj-$(CONFIG_TRACEPOINTS) += trace_portio.o > > So you are always enabling these if any CONFIG_TRACEPOINTS is enabled? > That seems brave. This doesn't enable the tracepoints. It just adds support for portio tracepoints to the kernel image. The tracepoints are disabled by default and must be explicitly enabled by the user at runtime. The overhead is very modest when portio tracing is disabled (as I mentioned above). > Again, you prevent any tracepoints from this code chunk, is that going > to be ok going forward? I addressed this question above. > Nit, "extern" isn't needed in .h files anymore. Not a big deal, just > for other work you do going forward. Noted. >> -#ifdef CONFIG_TRACEPOINTS >> +#if defined(CONFIG_TRACEPOINTS) && !defined(DISABLE_TRACEPOINTS) > > Why this global change? I addressed this question above. This is how we prevent tracepoint logic in header files from causing compiler errors in source files that aren't part of the kernel proper. > Anyway, it's up to the x86 maintainers now, good luck! > > But personally, I don't see the real need for this at all. It's a > debugging thing for what exactly? Who needs this? Who will use it? > When will they use it? And why? This comment confuses me. As you know I originally submitted a patch that added I/O tracing just to the 8250 serial driver. The patch was titled "create debugfs interface for UART register tracing". You said this at the time: "Anyway, again, cool feature, I like it, but if you can tie it into the existing trace framework better (either by using that entirely which might be best), or at the least, putting your hook into the data path with it, that would be best." My original patch went through a few revisions before Andy Shevchenko suggested I should add portio tracing instead in a manner similar to how CONFIG_TRACE_MMIO_ACCESS works. You agreed. Hence I created this patch.
