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

[Steven Rostedt]

On Mon, 23 Oct 2023 15:29:53 -0600
Dan Raymond <raymod2@xxxxxxxxx> wrote:

> On 10/21/2023 2:15 PM, Steven Rostedt wrote:
> 
> >> 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?  

BTW, trimming is good, but it's still better to leave the code that the
comment was talking about. I had to go back to my sent folder to figure it
out.

Adding back here for reference:


> > --- a/arch/x86/include/asm/cmpxchg_32.h
> > +++ b/arch/x86/include/asm/cmpxchg_32.h
> > @@ -37,13 +37,16 @@ static inline void set_64bit(volatile u64 *ptr, u64 value)
> >  
> >  #ifdef CONFIG_X86_CMPXCHG64
> >  #define arch_cmpxchg64(ptr, o, n)					\
> > -	((__typeof__(*(ptr)))__cmpxchg64((ptr), (unsigned long long)(o), \
> > +	((__typeof__(*(ptr)))__cmpxchg64((unsigned long long *)(ptr),	\
> > +					 (unsigned long long)(o),	\
> >  					 (unsigned long long)(n)))
> > -#define arch_cmpxchg64_local(ptr, o, n)					\
> > -	((__typeof__(*(ptr)))__cmpxchg64_local((ptr), (unsigned long long)(o), \
> > +#define arch_cmpxchg64_local(ptr, o, n)						\
> > +	((__typeof__(*(ptr)))__cmpxchg64_local((unsigned long long *)(ptr),	\
> > +					       (unsigned long long)(o),		\
> >  					       (unsigned long long)(n)))
> > -#define arch_try_cmpxchg64(ptr, po, n)					\
> > -	__try_cmpxchg64((ptr), (unsigned long long *)(po), \
> > +#define arch_try_cmpxchg64(ptr, po, n)			\
> > +	__try_cmpxchg64((unsigned long long *)(ptr),	\
> > +			(unsigned long long *)(po),	\
> >  			(unsigned long long)(n))
> >  #endif

> > 
> > Agreed, if this has issues, it probably should be a separate patch.  
> 
> As I mentioned to Greg, this fix is needed to avoid compiler warnings
> triggered by this patch.  If I submitted this separately it would have
> to be merged first.  Isn't it easier to combine them since this is
> not a functional change (it just makes a cast explicit)?

That's what patch series are for. You make a series of changes where one is
dependent on the other. But each commit should only do one thing. If you
need to fix something to do your one thing, that fix should be a separate
patch, but make it part of a series.

Just a "cast explicit" that's in generic code should be a separate change
with a change log explaining why it is needed, and why it didn't work
without it. Single commits can be just "no functional change".

> 
> >>> -#ifdef CONFIG_TRACEPOINTS
> >>> +#if defined(CONFIG_TRACEPOINTS) && !defined(DISABLE_TRACEPOINTS)    
> >>
> >> Why this global change?  
> > 
> > Yeah, DISABLE_TRACEPOINTS does not currently exist. If this is to be a
> > new way to disable TRACEPOINTS it needs a separate patch and be able to
> > disable tracepoints everywhere (maybe include/trace/*.h files also need
> > to be modified?), and also be documented somewhere in Documentation/trace.  
> 
> It's only needed to suppress compiler errors when building arch/x86/boot/*
> and arch/x86/realmode/*.  Those source files include various x86 headers
> such as <asm/msr.h> and <asm/shared/io.h>.  Those x86 headers include
> <linux/tracepoint-defs.h> which references static_key_false() in
> <linux/jump_label.h>.  DISABLE_TRACEPOINTS eliminates that reference and
> hence suppresses the compiler error.
> 
> I didn't intend for this macro to be used by developers adding new
> tracepoints so I didn't document it as such.  As far as creating a
> separate patch: again this is a requirement for this patch and it doesn't
> cause any functional changes so can't we combine them?

This is touching generic code, and as such, it *will* be used by others. If
generic code needs something like "DISABLE_TRACEPOINTS" for your use case,
it may be needed for someone else's.

For the same reason above, it really needs to be a separate patch with a
change log explaining why it is needed.

If this should only be used by code that is not part of the kernel proper,
then we should probably call it something else.

#ifdef PRE_LINUX_CODE  ?

Or something that more explicitly states that this is included in code
that's not part of the Linux proper. By saying "DISABLE_TRACEPOINTS" it
will look like this is the way to disable it for other use cases. Maybe we
want that, or perhaps we don't.

Either case, it should still be separate with a detailed explanation, for
when another developer sees this code, a git blame will give all the
explanation necessary for why it exists.

-- Steve