From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596AbbKLIbT (ORCPT ); Thu, 12 Nov 2015 03:31:19 -0500 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38613 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751428AbbKLIbR (ORCPT ); Thu, 12 Nov 2015 03:31:17 -0500 Date: Thu, 12 Nov 2015 09:31:13 +0100 From: Ingo Molnar To: Dave Hansen Cc: linux-kernel@vger.kernel.org, x86@kernel.org, dave.hansen@linux.intel.com, Linus Torvalds , Andy Lutomirski , Denys Vlasenko , Borislav Petkov , Thomas Gleixner , "H. Peter Anvin" , Fenghua Yu , Oleg Nesterov , Quentin Casasnovas Subject: Re: [RFC][PATCH] x86, fpu: trace points Message-ID: <20151112083113.GA15533@gmail.com> References: <20151110204424.268C1A2E@viggo.jf.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151110204424.268C1A2E@viggo.jf.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Dave Hansen wrote: > > From: Dave Hansen > > 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 > --- > > 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