RE: [PATCH] [LBR] Dump LBRs on Oops

["Berthier, Emmanuel" <emmanuel.berthier@intel.com>]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, November 26, 2014 2:08 PM
> To: Berthier, Emmanuel
> Cc: mingo@redhat.com; hpa@zytor.com; x86@kernel.org; Jarzmik, Robert;
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] [LBR] Dump LBRs on Oops
> 
> On Wed, 26 Nov 2014, Berthier, Emmanuel wrote:
> > > So, if perf uses LBR we do not print it? What a weird design decision.
> > > If the machine crashes, we want that information no matter whether
> > > perf is active or not. What kind of twisted logic is that?
> >
> > Ok, let me explain.
> > LBR usages are exclusive. Perf uses LBR to calculate some CPU
> > statistics. I use LBR to track code execution before Exceptions.
> > So, as soon as we enable perf, I disable LBR dump and vice versa.
> 
> That wants to be documented in the code.

Ok, will be in patch 2

> > > > +		} else if (x86_pmu.lbr_nr == 0) {
> > > > +			pr_cont(" (x86_model unknown, check
> > > intel_pmu_init())\n");
> > >
> > > Huch? Why we get here if the pmu does not support it at all? Why
> > > should we bother to print it? If it's not printed it's not available. It's that
> simple.
> >
> > That's a warning to point out that current core is not supported. New
> > cores have to be declared in
> >
> > intel_pmu_init() after:
> >
> > 	switch (boot_cpu_data.x86_model) {
> > 	. . .
> > 	case 28: /* Atom */
> > 	case 38: /* Lincroft */
> > 	case 39: /* Penwell */
> > 	case 53: /* Cloverview */
> > 	case 54: /* Cedarview */
> >
> > I work on new cores and their names will not be revealed before a while.
> > So, next time this feature will be used on a new core, it's important
> > to understand why is not supported and where to make the simple
> > update.
> 
> We add printks not for people who work on the support of unreleased
> hardware. They should better know what they are doing. If they can't figure
> that out they should not touch the kernel in the first place.

LoL
I'm part of those people, I've touched the kernel and I've figured out what was wrong.
And I would like to be helped next year for the next Core: I'm an old man and I need
to leave a white stone trail  ;-)
Could we agree on that one?

> > > >  void show_regs(struct pt_regs *regs)  {
> > > >  	int i;
> > > > @@ -314,10 +352,15 @@ void show_regs(struct pt_regs *regs)
> > > >  		unsigned char c;
> > > >  		u8 *ip;
> > > >
> > > > +		/*
> > > > +		 * Called before show_stack_log_lvl() as it could trig
> > > > +		 * page_fault and reenable LBR
> > >
> > > Huch? The kernel stack dump is going to page fault? If that happens
> > > then you are in deep shit anyway. I doubt that anything useful gets
> > > out of the machine at this point, LBR or not.
> > >
> > > Aside of that if we want to debug with the LBR then we better freeze
> > > that whole thing across a dump and be done with it.
> >
> > I met that case but did no dig deeply into it...
> 
> Hmm, a corrupted stack might trigger this together with some of the other
> debug options enabled. So we really might to put it in front.

Didn't catch you. Could you elaborate on that?

> > > > diff --git a/arch/x86/kernel/entry_64.S
> > > > b/arch/x86/kernel/entry_64.S index df088bb..120e989 100644
> > > > --- a/arch/x86/kernel/entry_64.S
> > > > +++ b/arch/x86/kernel/entry_64.S
> > > > @@ -1035,6 +1035,42 @@ apicinterrupt IRQ_WORK_VECTOR \
> > > >  	irq_work_interrupt smp_irq_work_interrupt  #endif
> > > >
> > > > +.macro STOP_LBR
> > > > +#ifdef CONFIG_LBR_DUMP_ON_EXCEPTION
> > > > +	testl $1, lbr_dump_on_exception
> > > > +	jz 1f
> > > > +	push %rax
> > > > +	push %rcx
> > > > +	push %rdx
> > > > +	movl $MSR_IA32_DEBUGCTLMSR, %ecx
> > > > +	rdmsr
> > > > +	and $~1, %eax	/* Disable LBR recording */
> > > > +	wrmsr
> > > > +	pop %rdx
> > > > +	pop %rcx
> > > > +	pop %rax
> > > > +1:
> > > > +#endif
> > > > +.endm
> > > > +
> > > > +.macro START_LBR
> > > > +#ifdef CONFIG_LBR_DUMP_ON_EXCEPTION
> > > > +	testl $1, lbr_dump_on_exception
> > > > +	jz 1f
> > > > +	push %rax
> > > > +	push %rcx
> > > > +	push %rdx
> > > > +	movl $MSR_IA32_DEBUGCTLMSR, %ecx
> > > > +	rdmsr
> > > > +	or $1, %eax		/* Enable LBR recording */
> 
> > So, by default, when Perf is not used, LBR will be enabled at the
> > first exception (usually a simple page fault) with default filtering
> > options, i.e no filtering.  As soon as we start perf, the
> > lbr_dump_on_exception global is unset and LBR start/stop are bypassed.
> >
> > LBR filtering is reset during perf stop.
> 
> So while I can see how this could be useful there are a few things which need
> more thought:
> 
> 1) We want to enable/disable this at boot time.
> 
>    In the disabled case we might also stub out the test/jz and replace
>    it by an unconditional jump, but that needs more thought.

I can add a cmdline option to disable it at boot time.
Do you propose to use code instruction patching (same as ftrace) also?
Is-it really worth to bypass test/jz as page fault handling is much more than few instructions?

> 2) Right now you stop the trace on every exception no matter whether
>    it comes from user or kernel space.
> 
>    Stopping the trace when we handle a user space fault does not make
>    any sense and inflicts just pointless overhead.
>
>    Aside of that if the fault handler then crashes we do not have the
>    LBR information because we froze it when entering from user space
>    in the first place.

Agree, but the LBR buffer contains only 8 records: we have to stop it as soon as possible.
If we add some test/jump/call before stopping it, relevant info will be flushed out.

Thanks.

> Thanks,
> 
> 	tglx
>