[RFC][PATCH] x86, fpu: trace points

[RFC][PATCH] x86, fpu: trace points

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

I've been carrying this patch around for a bit and it's helped me
solve at least a couple FPU-related issues.  It's a _bit_ of a
hack and probably too indiscriminate for mainline.

But, I'd be really interested to get something similar in to
mainline.

How do folks feel about this as it stands?  Could we do something
more structured?

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/fpu/internal.h |    5 +
 b/arch/x86/include/asm/trace/fpu.h    |  115 ++++++++++++++++++++++++++++++++++
 b/arch/x86/kernel/fpu/core.c          |   18 +++++
 b/arch/x86/kernel/fpu/signal.c        |    2 
 b/arch/x86/kvm/x86.c                  |    6 -
 5 files changed, 143 insertions(+), 3 deletions(-)

diff -puN /dev/null arch/x86/include/asm/trace/fpu.h
--- /dev/null	2015-07-13 14:24:11.435656502 -0700
+++ b/arch/x86/include/asm/trace/fpu.h	2015-11-10 12:43:24.809347944 -0800
@@ -0,0 +1,115 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fpu
+
+#if !defined(_TRACE_FPU_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_FPU_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(fpu_state_event,
+
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu),
+
+	TP_STRUCT__entry(
+		__field(struct fpu *, fpu)
+		__field(bool, fpregs_active)
+		__field(bool, fpstate_active)
+		__field(int, counter)
+		__field(u64, xfeatures)
+		__field(u64, xcomp_bv)
+	),
+
+	TP_fast_assign(
+		__entry->fpu = fpu;
+		__entry->fpregs_active = fpu->fpregs_active;
+		__entry->fpstate_active = fpu->fpstate_active;
+		__entry->counter = fpu->counter;
+		if (cpu_has_xsave) {
+			__entry->xfeatures = fpu->state.xsave.header.xfeatures;
+			__entry->xcomp_bv  = fpu->state.xsave.header.xcomp_bv;
+		}
+	),
+	TP_printk("fpu: %p fpregs_active: %d fpstate_active: %d counter: %d xfeatures: %llx xcomp_bv: %llx",
+		__entry->fpu,
+		__entry->fpregs_active,
+		__entry->fpstate_active,
+		__entry->counter,
+		__entry->xfeatures,
+		__entry->xcomp_bv
+	)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_state,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_before_save,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_after_save,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_before_restore,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_after_restore,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_regs_activated,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_regs_deactivated,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_activate_state,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_deactivate_state,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_init_state,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_dropped,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_copy_src,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+DEFINE_EVENT(fpu_state_event, fpu_copy_dst,
+	TP_PROTO(struct fpu *fpu),
+	TP_ARGS(fpu)
+);
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH asm/trace/
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE fpu
+#endif /* _TRACE_FPU_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff -puN arch/x86/kernel/fpu/core.c~fpu-trace-0 arch/x86/kernel/fpu/core.c
--- a/arch/x86/kernel/fpu/core.c~fpu-trace-0	2015-11-10 12:39:05.188577854 -0800
+++ b/arch/x86/kernel/fpu/core.c	2015-11-10 12:40:14.009697934 -0800
@@ -12,6 +12,9 @@
 
 #include <linux/hardirq.h>
 
+#define CREATE_TRACE_POINTS
+#include <asm/trace/fpu.h>
+
 /*
  * Represents the initial FPU state. It's mostly (but not completely) zeroes,
  * depending on the FPU hardware format:
@@ -188,10 +191,12 @@ void fpu__save(struct fpu *fpu)
 	WARN_ON_FPU(fpu != &current->thread.fpu);
 
 	preempt_disable();
+	trace_fpu_before_save(fpu);
 	if (fpu->fpregs_active) {
 		if (!copy_fpregs_to_fpstate(fpu))
 			fpregs_deactivate(fpu);
 	}
+	trace_fpu_after_save(fpu);
 	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(fpu__save);
@@ -273,6 +278,9 @@ int fpu__copy(struct fpu *dst_fpu, struc
 	if (src_fpu->fpstate_active && cpu_has_fpu)
 		fpu_copy(dst_fpu, src_fpu);
 
+	trace_fpu_copy_src(src_fpu);
+	trace_fpu_copy_dst(dst_fpu);
+
 	return 0;
 }
 
@@ -286,7 +294,9 @@ void fpu__activate_curr(struct fpu *fpu)
 
 	if (!fpu->fpstate_active) {
 		fpstate_init(&fpu->state);
+		trace_fpu_init_state(fpu);
 
+		trace_fpu_activate_state(fpu);
 		/* Safe to do for the current task: */
 		fpu->fpstate_active = 1;
 	}
@@ -312,7 +322,9 @@ void fpu__activate_fpstate_read(struct f
 	} else {
 		if (!fpu->fpstate_active) {
 			fpstate_init(&fpu->state);
+			trace_fpu_init_state(fpu);
 
+			trace_fpu_activate_state(fpu);
 			/* Safe to do for current and for stopped child tasks: */
 			fpu->fpstate_active = 1;
 		}
@@ -345,7 +357,9 @@ void fpu__activate_fpstate_write(struct
 		fpu->last_cpu = -1;
 	} else {
 		fpstate_init(&fpu->state);
+		trace_fpu_init_state(fpu);
 
+		trace_fpu_activate_state(fpu);
 		/* Safe to do for stopped child tasks: */
 		fpu->fpstate_active = 1;
 	}
@@ -367,9 +381,11 @@ void fpu__restore(struct fpu *fpu)
 
 	/* Avoid __kernel_fpu_begin() right after fpregs_activate() */
 	kernel_fpu_disable();
+	trace_fpu_before_restore(fpu);
 	fpregs_activate(fpu);
 	copy_kernel_to_fpregs(&fpu->state);
 	fpu->counter++;
+	trace_fpu_after_restore(fpu);
 	kernel_fpu_enable();
 }
 EXPORT_SYMBOL_GPL(fpu__restore);
@@ -398,6 +414,8 @@ void fpu__drop(struct fpu *fpu)
 
 	fpu->fpstate_active = 0;
 
+	trace_fpu_dropped(fpu);
+
 	preempt_enable();
 }
 
diff -puN arch/x86/include/asm/fpu/internal.h~fpu-trace-0 arch/x86/include/asm/fpu/internal.h
--- a/arch/x86/include/asm/fpu/internal.h~fpu-trace-0	2015-11-10 12:39:05.193578081 -0800
+++ b/arch/x86/include/asm/fpu/internal.h	2015-11-10 12:39:05.208578761 -0800
@@ -17,6 +17,7 @@
 #include <asm/user.h>
 #include <asm/fpu/api.h>
 #include <asm/fpu/xstate.h>
+#include <asm/trace/fpu.h>
 
 /*
  * High level FPU state handling functions:
@@ -527,6 +528,7 @@ static inline void __fpregs_deactivate(s
 
 	fpu->fpregs_active = 0;
 	this_cpu_write(fpu_fpregs_owner_ctx, NULL);
+	trace_fpu_regs_deactivated(fpu);
 }
 
 /* Must be paired with a 'clts' (fpregs_activate_hw()) before! */
@@ -536,6 +538,7 @@ static inline void __fpregs_activate(str
 
 	fpu->fpregs_active = 1;
 	this_cpu_write(fpu_fpregs_owner_ctx, fpu);
+	trace_fpu_regs_activated(fpu);
 }
 
 /*
@@ -606,11 +609,13 @@ switch_fpu_prepare(struct fpu *old_fpu,
 
 		/* But leave fpu_fpregs_owner_ctx! */
 		old_fpu->fpregs_active = 0;
+		trace_fpu_regs_deactivated(old_fpu);
 
 		/* Don't change CR0.TS if we just switch! */
 		if (fpu.preload) {
 			new_fpu->counter++;
 			__fpregs_activate(new_fpu);
+			trace_fpu_regs_activated(new_fpu);
 			prefetch(&new_fpu->state);
 		} else {
 			__fpregs_deactivate_hw();
diff -puN arch/x86/kernel/fpu/signal.c~fpu-trace-0 arch/x86/kernel/fpu/signal.c
--- a/arch/x86/kernel/fpu/signal.c~fpu-trace-0	2015-11-10 12:39:05.195578171 -0800
+++ b/arch/x86/kernel/fpu/signal.c	2015-11-10 12:39:05.208578761 -0800
@@ -10,6 +10,7 @@
 #include <asm/fpu/regset.h>
 
 #include <asm/sigframe.h>
+#include <asm/trace/fpu.h>
 
 static struct _fpx_sw_bytes fx_sw_reserved, fx_sw_reserved_ia32;
 
@@ -311,6 +312,7 @@ static int __fpu__restore_sig(void __use
 		if (__copy_from_user(&fpu->state.xsave, buf_fx, state_size) ||
 		    __copy_from_user(&env, buf, sizeof(env))) {
 			fpstate_init(&fpu->state);
+			trace_fpu_init_state(fpu);
 			err = -1;
 		} else {
 			sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
diff -puN arch/x86/kvm/x86.c~fpu-trace-0 arch/x86/kvm/x86.c
--- a/arch/x86/kvm/x86.c~fpu-trace-0	2015-11-10 12:39:05.202578489 -0800
+++ b/arch/x86/kvm/x86.c	2015-11-10 12:39:05.211578897 -0800
@@ -55,9 +55,6 @@
 #include <linux/irqbypass.h>
 #include <trace/events/kvm.h>
 
-#define CREATE_TRACE_POINTS
-#include "trace.h"
-
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/desc.h>
@@ -68,6 +65,9 @@
 #include <asm/div64.h>
 #include <asm/irq_remapping.h>
 
+#define CREATE_TRACE_POINTS
+#include "trace.h"
+
 #define MAX_IO_MSRS 256
 #define KVM_MAX_MCE_BANKS 32
 #define KVM_MCE_CAP_SUPPORTED (MCG_CTL_P | MCG_SER_P)
_

Re: [RFC][PATCH] x86, fpu: trace points

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> I've been carrying this patch around for a bit and it's helped me
> solve at least a couple FPU-related issues.  It's a _bit_ of a
> hack and probably too indiscriminate for mainline.
> 
> But, I'd be really interested to get something similar in to
> mainline.
> 
> How do folks feel about this as it stands?  Could we do something
> more structured?
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/include/asm/fpu/internal.h |    5 +
>  b/arch/x86/include/asm/trace/fpu.h    |  115 ++++++++++++++++++++++++++++++++++
>  b/arch/x86/kernel/fpu/core.c          |   18 +++++
>  b/arch/x86/kernel/fpu/signal.c        |    2 
>  b/arch/x86/kvm/x86.c                  |    6 -
>  5 files changed, 143 insertions(+), 3 deletions(-)

It certainly looks good to me!

Which bit do you consider a hack? It's a pretty straightforward set of 
tracepoints.

> +DECLARE_EVENT_CLASS(fpu_state_event,
> +
> +	TP_PROTO(struct fpu *fpu),
> +	TP_ARGS(fpu),
> +
> +	TP_STRUCT__entry(
> +		__field(struct fpu *, fpu)
> +		__field(bool, fpregs_active)
> +		__field(bool, fpstate_active)
> +		__field(int, counter)
> +		__field(u64, xfeatures)
> +		__field(u64, xcomp_bv)
> +	),

The only detail I'd change is that I'd make the tracepoint names explicitly 
x86-ish, i.e. I'd rename the event class to 'x86_fpu'. (No need to put 
'state_event' into the class name, all tracepoints are events and we obviously 
trace some sort of state.)

Likewise I'd name the tracepoints themselves along the 'x86_fpu_*' pattern.

> +	TP_fast_assign(
> +		__entry->fpu = fpu;
> +		__entry->fpregs_active = fpu->fpregs_active;
> +		__entry->fpstate_active = fpu->fpstate_active;
> +		__entry->counter = fpu->counter;

Nit: my pet peeve about vertically aligning initializations, like you did it just 
a bit further down:

> +		if (cpu_has_xsave) {
> +			__entry->xfeatures = fpu->state.xsave.header.xfeatures;
> +			__entry->xcomp_bv  = fpu->state.xsave.header.xcomp_bv;
> +		}
> +	),
> +	TP_printk("fpu: %p fpregs_active: %d fpstate_active: %d counter: %d xfeatures: %llx xcomp_bv: %llx",

... and here I'd make the trace message "x86/fpu: ", to make it obvious and easy 
to parse. The events are very x86 specific in any case.

Thanks,

	Ingo