* Dave Hansen <d...@sr71.net> wrote:
>
> From: Dave Hansen <dave.han...@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.han...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
