Message-ID: <65CD3FC07F3BF942ABE211646D72D770356ED8F4@IRSMSX110.ger.corp.intel.com>
Date:	Fri, 5 Dec 2014 13:14:21 +0000
From:	"Berthier, Emmanuel" <emmanuel.berthier@...el.com>
To:	Andy Lutomirski <luto@...capital.net>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
	"Jarzmik, Robert" <robert.jarzmik@...el.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] [LBR] Dump LBRs on Exception

> From: Andy Lutomirski [mailto:luto@...capital.net]
> Sent: Thursday, December 4, 2014 7:10 PM
> To: Berthier, Emmanuel
> Cc: Thomas Gleixner; H. Peter Anvin; X86 ML; Jarzmik, Robert; LKML
> Subject: Re: [PATCH v2] [LBR] Dump LBRs on Exception
> >> And the general problem with this approach (even ignoring the
> >> performance hit, and kernel faults on user addresses really do happen
> >> in real workloads) is that you're not saving and restoring
> MSR_IA32_DEBUGCTL.
> >> It may be that
> >> the rest of your patch does whatever magic is needed to make this
> >> work, but from just this code it's not at all obvious that this is correct.
> >
> > The algorithm is quite simple:
> > When I enter in Exception handler, I stop LBR recording, and dump its
> content later if needed.
> > When I leave Exception Handler, I restart LBR recording.
> > So, after the first exception, LBR in On.
> > In case of nested Exceptions and crash, you're right, LBR will probably not
> be relevant.
> > But your proposal does not solve this issue: If we save registers
> > during 1rst exception, and then overwrite them during 2nd level, we will
> lose relevant info if crash is due to the 1rst exception.
> >
> >> Hence my suggestion for rdmsr -- if you're willing to enable this and
> >> take the performance hit, you can simplify it a lot and save some
> >> branch slots by unconditionally doing the rdmsrs if you've enabled the LBR
> tracing IDT entry.
> >> The simplification from using rdmsr isn't that the save code is
> >> simplified -- it's that there's no state change on exception entry,
> >> so you don't need to worry about restoring state correctly on the way out
> or during a context switch.
> >> And you can enable/disable the whole thing just by writing to the
> >> IDT, so there's no performance hit at all in the disabled case.
> >
> > Concerning performances: if it's really matter, the better is to disable the
> CONFIG switch.
> > But if we enable it, it's for using it I guess, and in that case, bypassing
> UserSpace page faults is better.
> > You're proposal of "unconditionally doing the rdmsrs" is not good in that
> case.
> > The only small gain is when CONFIG is enable and feature is disabled by
> cmdline:
> > - with my proposal, we get 1 test and 1 jmp more (if I switch Kernel test
> with LBR state test): for an exception treatment, does it really matter?
> >
> > We can mix our proposals: keep my STOP/START code, and replace the
> dynamic disabling test by IDT change.
> > I hope the code will stay readable.
> > Do we really want to save 2 instructions?
> 
> I don't really care about the number of instructions. 

Ok.

> But there are still all the nasty cases:
> 
>  - Context switch during exception processing (both in the C handler and in
> the retint code).
>  - PMI during exception processing.
>  - Exception while perf is poking at LBR msrs.

For Perf compatibility: we disable LBR dump feature when Perf is using LBR.
For other cases, they are rare, but real. The current implementation does not
break anything, only the LBR dump content will be irrelevant.

> Where are you planning on saving the start/stop previous state?

To handle nested exceptions or context switch, we need to store LBRs into the stack.
And as LBRs number is linked to arch, it will be funny.
In fact, I have evaluated this solution during the study, and gave it up after looking at the impact.

Maybe someone has a better proposal?


Thanks,

Emmmanuel.

Powered by blists - more mailing lists