Re: [PATCH v5] arch/x86: port I/O tracing on x86

[Dan Raymond]

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.