Add two trace points x86_fpu_latency_xsave and x86_fpu_latency_xrstor.
The latency dumped by the new trace points can tell when XSAVES/XRSTORS
are getting more or less expensive, and get out the RFBM
(requested-feature bitmap) and XINUSE to figure out the reason.

Calculate the latency of instructions XSAVES and XRSTORS within a
single trace event respectively. Another option considered was to
have 2 separated trace events marking the start and finish of the
XSAVES/XRSTORS. The latency was calculated from the 2 trace points in
user space, but there was significant overhead added by the trace
function itself.

In internal testing, the single trace point option which is
implemented here proved to save big overhead introduced by trace
function.

Make use of trace_clock() to calculate the latency, which is based on
cpu_clock() with precision at most ~1 jiffy between CPUs.

CONFIG_X86_DEBUG_FPU and CONFIG_TRACEPOINTS are required. And the
compiler will get rid of all the extra crust when either of the two
configs is disabled.

If both of the configs are enabled, xsave/xrstor_tracing_enabled
would be reduced to a static check for tracing enabled. Thus, in the
fast path there would be only 2 additional static checks.

Since trace points can be enabled dynamically, while the code is
checking tracepoint_enabled(trace_event), the trace_event could be
concurrently enabled. Hence there is probability to get single once
noisy result 'trace_clock() - (-1)' at the moment enabling the trace
points x86_fpu_latency_*. Leave the noise here instead of additional
conditions while calling the x86_fpu_latency_* because it's not worth
for the only once noise. It's easy to filter out by the following
consuming script or other user space tool.

Trace log looks like following:
  x86_fpu_latency_xsave: x86/fpu: latency:100 RFBM:0x202e7 XINUSE:0x202
  x86_fpu_latency_xrstor: x86/fpu: latency:99 RFBM:0x202e7 XINUSE:0x202

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Yi Sun <yi.sun@intel.com>

diff --git a/arch/x86/include/asm/trace/fpu.h b/arch/x86/include/asm/trace/fpu.h
index 4645a6334063..5f7cb633df09 100644
--- a/arch/x86/include/asm/trace/fpu.h
+++ b/arch/x86/include/asm/trace/fpu.h
@@ -89,6 +89,41 @@ DEFINE_EVENT(x86_fpu, x86_fpu_xstate_check_failed,
 	TP_ARGS(fpu)
 );
 
+DECLARE_EVENT_CLASS(x86_fpu_latency,
+	TP_PROTO(struct fpstate *fpstate, u64 latency),
+	TP_ARGS(fpstate, latency),
+
+	TP_STRUCT__entry(
+		__field(struct fpstate *, fpstate)
+		__field(u64, latency)
+		__field(u64, rfbm)
+		__field(u64, xinuse)
+	),
+
+	TP_fast_assign(
+		__entry->fpstate = fpstate;
+		__entry->latency = latency;
+		__entry->rfbm = fpstate->xfeatures;
+		__entry->xinuse = fpstate->regs.xsave.header.xfeatures;
+	),
+
+	TP_printk("x86/fpu: latency:%lld RFBM:0x%llx XINUSE:0x%llx",
+		__entry->latency,
+		__entry->rfbm,
+		__entry->xinuse
+	)
+);
+
+DEFINE_EVENT(x86_fpu_latency, x86_fpu_latency_xsave,
+	TP_PROTO(struct fpstate *fpstate, u64 latency),
+	TP_ARGS(fpstate, latency)
+);
+
+DEFINE_EVENT(x86_fpu_latency, x86_fpu_latency_xrstor,
+	TP_PROTO(struct fpstate *fpstate, u64 latency),
+	TP_ARGS(fpstate, latency)
+);
+
 #undef TRACE_INCLUDE_PATH
 #define TRACE_INCLUDE_PATH asm/trace/
 #undef TRACE_INCLUDE_FILE
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index a4ecb04d8d64..5a8321d9f7db 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -5,6 +5,9 @@
 #include <asm/cpufeature.h>
 #include <asm/fpu/xstate.h>
 #include <asm/fpu/xcr.h>
+#include <asm/trace/fpu.h>
+
+#include <linux/trace_clock.h>
 
 #ifdef CONFIG_X86_64
 DECLARE_PER_CPU(u64, xfd_state);
@@ -68,6 +71,20 @@ static inline u64 xfeatures_mask_independent(void)
 	return XFEATURE_MASK_INDEPENDENT;
 }
 
+static inline bool xsave_tracing_enabled(void)
+{
+	if (!IS_ENABLED(CONFIG_X86_DEBUG_FPU))
+		return false;
+	return tracepoint_enabled(x86_fpu_latency_xsave);
+}
+
+static inline bool xrstor_tracing_enabled(void)
+{
+	if (!IS_ENABLED(CONFIG_X86_DEBUG_FPU))
+		return false;
+	return tracepoint_enabled(x86_fpu_latency_xrstor);
+}
+
 /* XSAVE/XRSTOR wrapper functions */
 
 #ifdef CONFIG_X86_64
@@ -113,7 +130,7 @@ static inline u64 xfeatures_mask_independent(void)
  * original instruction which gets replaced. We need to use it here as the
  * address of the instruction where we might get an exception at.
  */
-#define XSTATE_XSAVE(st, lmask, hmask, err)				\
+#define __XSTATE_XSAVE(st, lmask, hmask, err)				\
 	asm volatile(ALTERNATIVE_3(XSAVE,				\
 				   XSAVEOPT, X86_FEATURE_XSAVEOPT,	\
 				   XSAVEC,   X86_FEATURE_XSAVEC,	\
@@ -126,11 +143,22 @@ static inline u64 xfeatures_mask_independent(void)
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")
 
+#define XSTATE_XSAVE(fps, lmask, hmask, err)				\
+	do {								\
+		struct fpstate *f = fps;				\
+		u64 tc = -1;						\
+		if (xsave_tracing_enabled())				\
+			tc = trace_clock();				\
+		__XSTATE_XSAVE(&f->regs.xsave, lmask, hmask, err);	\
+		if (xsave_tracing_enabled())				\
+			trace_x86_fpu_latency_xsave(f, trace_clock() - tc);\
+	} while (0)
+
 /*
  * Use XRSTORS to restore context if it is enabled. XRSTORS supports compact
  * XSAVE area format.
  */
-#define XSTATE_XRESTORE(st, lmask, hmask)				\
+#define __XSTATE_XRESTORE(st, lmask, hmask)				\
 	asm volatile(ALTERNATIVE(XRSTOR,				\
 				 XRSTORS, X86_FEATURE_XSAVES)		\
 		     "\n"						\
@@ -140,6 +168,17 @@ static inline u64 xfeatures_mask_independent(void)
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")
 
+#define XSTATE_XRESTORE(fps, lmask, hmask)				\
+	do {								\
+		struct fpstate *f = fps;				\
+		u64 tc = -1;						\
+		if (xrstor_tracing_enabled())				\
+			tc = trace_clock();				\
+		__XSTATE_XRESTORE(&f->regs.xsave, lmask, hmask);	\
+		if (xrstor_tracing_enabled())				\
+			trace_x86_fpu_latency_xrstor(f, trace_clock() - tc);\
+	} while (0)
+
 #if defined(CONFIG_X86_64) && defined(CONFIG_X86_DEBUG_FPU)
 extern void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor);
 #else
@@ -184,7 +223,7 @@ static inline void os_xsave(struct fpstate *fpstate)
 	WARN_ON_FPU(!alternatives_patched);
 	xfd_validate_state(fpstate, mask, false);
 
-	XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
+	XSTATE_XSAVE(fpstate, lmask, hmask, err);
 
 	/* We should never fault when copying to a kernel buffer: */
 	WARN_ON_FPU(err);
@@ -201,7 +240,7 @@ static inline void os_xrstor(struct fpstate *fpstate, u64 mask)
 	u32 hmask = mask >> 32;
 
 	xfd_validate_state(fpstate, mask, true);
-	XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
+	XSTATE_XRESTORE(fpstate, lmask, hmask);
 }
 
 /* Restore of supervisor state. Does not require XFD */
@@ -211,7 +250,7 @@ static inline void os_xrstor_supervisor(struct fpstate *fpstate)
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
 
-	XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
+	XSTATE_XRESTORE(fpstate, lmask, hmask);
 }
 
 /*
-- 
2.34.1

* Yi Sun <yi.sun@intel.com> wrote:

> +#define XSTATE_XSAVE(fps, lmask, hmask, err)				\
> +	do {								\
> +		struct fpstate *f = fps;				\
> +		u64 tc = -1;						\
> +		if (xsave_tracing_enabled())				\
> +			tc = trace_clock();				\
> +		__XSTATE_XSAVE(&f->regs.xsave, lmask, hmask, err);	\
> +		if (xsave_tracing_enabled())				\
> +			trace_x86_fpu_latency_xsave(f, trace_clock() - tc);\
> +	} while (0)
> +
>  /*
>   * Use XRSTORS to restore context if it is enabled. XRSTORS supports compact
>   * XSAVE area format.
>   */
> -#define XSTATE_XRESTORE(st, lmask, hmask)				\
> +#define __XSTATE_XRESTORE(st, lmask, hmask)				\
>  	asm volatile(ALTERNATIVE(XRSTOR,				\
>  				 XRSTORS, X86_FEATURE_XSAVES)		\
>  		     "\n"						\
> @@ -140,6 +168,17 @@ static inline u64 xfeatures_mask_independent(void)
>  		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
>  		     : "memory")
>  
> +#define XSTATE_XRESTORE(fps, lmask, hmask)				\
> +	do {								\
> +		struct fpstate *f = fps;				\
> +		u64 tc = -1;						\
> +		if (xrstor_tracing_enabled())				\
> +			tc = trace_clock();				\
> +		__XSTATE_XRESTORE(&f->regs.xsave, lmask, hmask);	\
> +		if (xrstor_tracing_enabled())				\
> +			trace_x86_fpu_latency_xrstor(f, trace_clock() - tc);\
> +	} while (0)
> +
>  #if defined(CONFIG_X86_64) && defined(CONFIG_X86_DEBUG_FPU)
>  extern void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor);
>  #else
> @@ -184,7 +223,7 @@ static inline void os_xsave(struct fpstate *fpstate)
>  	WARN_ON_FPU(!alternatives_patched);
>  	xfd_validate_state(fpstate, mask, false);
>  
> -	XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
> +	XSTATE_XSAVE(fpstate, lmask, hmask, err);
>  
>  	/* We should never fault when copying to a kernel buffer: */
>  	WARN_ON_FPU(err);
> @@ -201,7 +240,7 @@ static inline void os_xrstor(struct fpstate *fpstate, u64 mask)
>  	u32 hmask = mask >> 32;
>  
>  	xfd_validate_state(fpstate, mask, true);
> -	XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
> +	XSTATE_XRESTORE(fpstate, lmask, hmask);
>  }

Instead of adding overhead to the regular FPU context saving/restoring code 
paths, could you add a helper function that has tracing code included, but 
which isn't otherwise used - and leave the regular code with no tracing 
overhead?

This puts a bit of a long-term maintenance focus on making sure that the 
traced functionality won't bitrot, but I'd say that's preferable to adding 
tracing overhead.

Thanks,

	Ingo

On 02.09.2023 12:49, Ingo Molnar wrote:
>
>* Yi Sun <yi.sun@intel.com> wrote:
>
>> +#define XSTATE_XSAVE(fps, lmask, hmask, err)				\
>> +	do {								\
>> +		struct fpstate *f = fps;				\
>> +		u64 tc = -1;						\
>> +		if (xsave_tracing_enabled())				\
>> +			tc = trace_clock();				\
>> +		__XSTATE_XSAVE(&f->regs.xsave, lmask, hmask, err);	\
>> +		if (xsave_tracing_enabled())				\
>> +			trace_x86_fpu_latency_xsave(f, trace_clock() - tc);\
>> +	} while (0)
>> +
>>  /*
>>   * Use XRSTORS to restore context if it is enabled. XRSTORS supports compact
>>   * XSAVE area format.
>>   */
>> -#define XSTATE_XRESTORE(st, lmask, hmask)				\
>> +#define __XSTATE_XRESTORE(st, lmask, hmask)				\
>>  	asm volatile(ALTERNATIVE(XRSTOR,				\
>>  				 XRSTORS, X86_FEATURE_XSAVES)		\
>>  		     "\n"						\
>> @@ -140,6 +168,17 @@ static inline u64 xfeatures_mask_independent(void)
>>  		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
>>  		     : "memory")
>>
>> +#define XSTATE_XRESTORE(fps, lmask, hmask)				\
>> +	do {								\
>> +		struct fpstate *f = fps;				\
>> +		u64 tc = -1;						\
>> +		if (xrstor_tracing_enabled())				\
>> +			tc = trace_clock();				\
>> +		__XSTATE_XRESTORE(&f->regs.xsave, lmask, hmask);	\
>> +		if (xrstor_tracing_enabled())				\
>> +			trace_x86_fpu_latency_xrstor(f, trace_clock() - tc);\
>> +	} while (0)
>> +
>>  #if defined(CONFIG_X86_64) && defined(CONFIG_X86_DEBUG_FPU)
>>  extern void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor);
>>  #else
>> @@ -184,7 +223,7 @@ static inline void os_xsave(struct fpstate *fpstate)
>>  	WARN_ON_FPU(!alternatives_patched);
>>  	xfd_validate_state(fpstate, mask, false);
>>
>> -	XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
>> +	XSTATE_XSAVE(fpstate, lmask, hmask, err);
>>
>>  	/* We should never fault when copying to a kernel buffer: */
>>  	WARN_ON_FPU(err);
>> @@ -201,7 +240,7 @@ static inline void os_xrstor(struct fpstate *fpstate, u64 mask)
>>  	u32 hmask = mask >> 32;
>>
>>  	xfd_validate_state(fpstate, mask, true);
>> -	XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
>> +	XSTATE_XRESTORE(fpstate, lmask, hmask);
>>  }
>
>Instead of adding overhead to the regular FPU context saving/restoring code
>paths, could you add a helper function that has tracing code included, but
>which isn't otherwise used - and leave the regular code with no tracing
>overhead?
>
>This puts a bit of a long-term maintenance focus on making sure that the
>traced functionality won't bitrot, but I'd say that's preferable to adding
>tracing overhead.
>
Hi Ingo,
It is actually a helper function, and I have renamed the original function
to __XSTATE_XSAVE. This function is only used in this particular context.

Furthermore, according doc static-keys.txt, the condition
xrstor_tracing_enabled() would introduce only a minimal overhead when the
trace is disabled. I believe it is a negligible impact on the performance
when the trace is disabled.

Please let me know if you have any questions or concerns about this function.

Thanks
   --Sun, Yi

* Yi Sun <yi.sun@intel.com> wrote:

> > Instead of adding overhead to the regular FPU context saving/restoring 
> > code paths, could you add a helper function that has tracing code 
> > included, but which isn't otherwise used - and leave the regular code 
> > with no tracing overhead?

> Furthermore, according doc static-keys.txt, the condition 
> xrstor_tracing_enabled() would introduce only a minimal overhead when the 
> trace is disabled. I believe it is a negligible impact on the performance 
> when the trace is disabled.

Why introduce *any* extra overhead if it's possible to test the 
functionality separately? The stated goal of the series is only to measure 
FPU context switch performance, which doesn't require extra added overhead 
to the actual context switch path.

[ Or if you want to convince reviewers that the overhead is indeed minimal, 
  please provide before/after generated assembly of the affected code that 
  demonstrates minimal impact. ]

Thanks,

	Ingo

> Instead of adding overhead to the regular FPU context saving/restoring code 
> paths, could you add a helper function that has tracing code included, but 
> which isn't otherwise used - and leave the regular code with no tracing 
> overhead?
> 
> This puts a bit of a long-term maintenance focus on making sure that the 
> traced functionality won't bitrot, but I'd say that's preferable to adding 
> tracing overhead.

Or just use PT

% sudo perf record --kcore -e intel_pt/cyc=1,cyc_thresh=1/k --filter 'filter save_fpregs_to_fpstate' -a sleep 5
% sudo perf script --insn-trace --xed -F -comm,-tid,-dso,-sym,-symoff,+ipc
[000] 677203.751913565:  ffffffffa7046230               nopw  %ax, (%rax)
[000] 677203.751913565:  ffffffffa7046234               nopl  %eax, (%rax,%rax,1)
[000] 677203.751913565:  ffffffffa7046239               mov %rdi, %rcx
[000] 677203.751913565:  ffffffffa704623c               nopl  %eax, (%rax,%rax,1)
[000] 677203.751913565:  ffffffffa7046241               movq
0x10(%rdi), %rsi
[000] 677203.751913565:  ffffffffa7046245               movq  0x8(%rsi), %rax
[000] 677203.751913565:  ffffffffa7046249               leaq 0x40(%rsi), %rdi
[000] 677203.751913565:  ffffffffa704624d               mov %rax, %rdx
[000] 677203.751913565:  ffffffffa7046250               shr $0x20, %rdx
[000] 677203.751913565:  ffffffffa7046254               xsaves64 (%rdi)
[000] 677203.751913565:  ffffffffa7046258               xor %edi, %edi
[000] 677203.751913565:  ffffffffa704625a               movq 0x10(%rcx), %rax
[000] 677203.751913565:  ffffffffa704625e               testb  $0xc0, 0x240(%rax)
[000] 677203.751913636:  ffffffffa7046265               jz 0xffffffffa7046285    IPC: 0.16 (14/85)
...


So it took 85 cycles here.

(it includes a few extra instructions, but I bet they're less than what
ftrace adds. This example is for XSAVE, but can be similarly extended
for XRSTOR)

-Andi

On 02.09.2023 12:09, Andi Kleen wrote:
>> Instead of adding overhead to the regular FPU context saving/restoring code
>> paths, could you add a helper function that has tracing code included, but
>> which isn't otherwise used - and leave the regular code with no tracing
>> overhead?
>>

>> This puts a bit of a long-term maintenance focus on making sure that the
>> traced functionality won't bitrot, but I'd say that's preferable to adding
>> tracing overhead.
>
>Or just use PT
>
>% sudo perf record --kcore -e intel_pt/cyc=1,cyc_thresh=1/k --filter 'filter save_fpregs_to_fpstate' -a sleep 5
>% sudo perf script --insn-trace --xed -F -comm,-tid,-dso,-sym,-symoff,+ipc
>[000] 677203.751913565:  ffffffffa7046230               nopw  %ax, (%rax)
>[000] 677203.751913565:  ffffffffa7046234               nopl  %eax, (%rax,%rax,1)
>[000] 677203.751913565:  ffffffffa7046239               mov %rdi, %rcx
>[000] 677203.751913565:  ffffffffa704623c               nopl  %eax, (%rax,%rax,1)
>[000] 677203.751913565:  ffffffffa7046241               movq
>0x10(%rdi), %rsi
>[000] 677203.751913565:  ffffffffa7046245               movq  0x8(%rsi), %rax
>[000] 677203.751913565:  ffffffffa7046249               leaq 0x40(%rsi), %rdi
>[000] 677203.751913565:  ffffffffa704624d               mov %rax, %rdx
>[000] 677203.751913565:  ffffffffa7046250               shr $0x20, %rdx
>[000] 677203.751913565:  ffffffffa7046254               xsaves64 (%rdi)
>[000] 677203.751913565:  ffffffffa7046258               xor %edi, %edi
>[000] 677203.751913565:  ffffffffa704625a               movq 0x10(%rcx), %rax
>[000] 677203.751913565:  ffffffffa704625e               testb  $0xc0, 0x240(%rax)
>[000] 677203.751913636:  ffffffffa7046265               jz 0xffffffffa7046285    IPC: 0.16 (14/85)
>...
>
>
>So it took 85 cycles here.
>
>(it includes a few extra instructions, but I bet they're less than what
>ftrace adds. This example is for XSAVE, but can be similarly extended
>for XRSTOR)
>
Hi Andi,
Thank you for your guidance on Intel PT.

I recall that we have discussed this topic via email before.
I have compared the two methods that calculate the latency:
  1. Calculate using perf-intel-pt with functions filter.
  2. Calculate the tsc delta explicitly in kernel, and dump the delta by a
  single trace point as what this patch does.

My findings are:
  1. Intel-pt is the most accurate method, but it's likely just a one-time
  exercise because 'filter with function' requires rebuilding the kernel
  and changing the definition of functions 'os_xsave' and 'os_xrstor' into
  'noinline' instead of 'inline'.
  2. I collected the latency data with the two methods, and the method in
  this patch can achieve results that are close to those with intel-pt.
  And it only introduces a negligible impact on the performance when the
  trace is disabled, as I explained to Ingo earlier.
  
Hope this clarifies my approach. We're using this patch set to do tests
on Intel's new brand chipsets.

Thanks
    --Sun, Yi

On 9/6/23 02:18, Yi Sun wrote:
> Or just use PT

I'd really like to be able to use this mechanism across a wide range of
systems over time and vendors.  For instance, if Intel's AVX512 XSAVE
implementation is much faster than AMD's, it would be nice to show some
apples-to-apples data to motivate AMD to do better.  We can't do that
with PT.

* Dave Hansen <dave.hansen@intel.com> wrote:

> On 9/6/23 02:18, Yi Sun wrote:
> > Or just use PT
> 
> I'd really like to be able to use this mechanism across a wide range of 
> systems over time and vendors.  For instance, if Intel's AVX512 XSAVE 
> implementation is much faster than AMD's, it would be nice to show some 
> apples-to-apples data to motivate AMD to do better.  We can't do that 
> with PT.

Ack - and with the explicit tooling support, it's also very easy to provide 
such numbers.

As long as the regular FPU code paths do not get new tracing overhead 
added, this looks like a useful tool.

Thanks,

	Ingo

On 07.09.2023 00:02, Ingo Molnar wrote:
>
>* Dave Hansen <dave.hansen@intel.com> wrote:
>
>> On 9/6/23 02:18, Yi Sun wrote:
>> > Or just use PT
>>
>> I'd really like to be able to use this mechanism across a wide range of
>> systems over time and vendors.  For instance, if Intel's AVX512 XSAVE
>> implementation is much faster than AMD's, it would be nice to show some
>> apples-to-apples data to motivate AMD to do better.  We can't do that
>> with PT.
>
>Ack - and with the explicit tooling support, it's also very easy to provide
>such numbers.
>
>As long as the regular FPU code paths do not get new tracing overhead
>added, this looks like a useful tool.
>
>Thanks,
>
>	Ingo
Hi Ingo and Dave,
We have been running the tool since kernel v5.19, and the patch set has
been tested in our internal repository for several months.
Additionally, we have implemented a test suite of micro-benchmark in user
space that works with this kernel trace, and it will be open-sourced soon.
With all these tools, we have obtained useful data, and we haven't
encountered any issues.

It would be great for the quality of X86 SIMD instructions if the patch
could catch up kernel v6.6.

Thanks
   --Sun, Yi