Re: 8bf00a529967dafbbb210b377c38a15834d1e979 - performance regression?

[Gleb Natapov <gleb@redhat.com>]

On Tue, Nov 05, 2013 at 12:18:57PM +0200, Michael S. Tsirkin wrote:
> On Mon, Nov 04, 2013 at 10:44:43PM +0200, Gleb Natapov wrote:
> > On Mon, Nov 04, 2013 at 10:33:57PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Nov 04, 2013 at 10:13:39PM +0200, Gleb Natapov wrote:
> > > > On Mon, Nov 04, 2013 at 10:11:33PM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Oct 31, 2013 at 09:48:08AM +0200, Gleb Natapov wrote:
> > > > > > On Thu, Oct 31, 2013 at 02:21:46AM +0200, Michael S. Tsirkin wrote:
> > > > > > > commit 8bf00a529967dafbbb210b377c38a15834d1e979:
> > > > > > > "    KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
> > > > > > > as far as I can tell supposed to bring about performance improvement
> > > > > > > on hardware that supports it?
> > > > > > No, it (and commits after it) supposed to fix a bug which it did.
> > > > > > 
> > > > > > > Instead it seems to make the typical case (not running guest
> > > > > > > under perf) a bit slower than it used to be.
> > > > > > > the cost of VMexit goes up by about 50 cycles
> > > > > > > on sandy bridge where the optimization in question
> > > > > > > actually is activated.
> > > > > > >
> > > > > > You seams to be confused. 8bf00a529967dafbbb210 adds support for special
> > > > > > PERF_GLOBAL_CTRL switching, but does not add code to switch anything,
> > > > > > so the commit itself is a nop.
> > > > > 
> > > > > It does add code to add_atomic_switch_msr.
> > > > > 
> > > > So what? You do not read what I wrote.
> > > 
> > > 
> > > It's simple: if I revert 8bf00a529967dafbbb210 then exit latency
> > > is reduced.
> > > You seem to tell me it should be a nop, but in practice it isn't.
> > > 
> > 
> > No, if you read below I am saying that it looks like you are claiming that
> > generic msr switch mechanism is faster and I am not buying that. If you
> > believe this to be the case ask Intel for explanation. Your claim about
> > "not running guest under perf" is even stranger since in this case no msr
> > switch should happen regardless of the aforementioned commit (unless guest
> > or host runs nmi watchdog, but then switch will happen no matter if perf
> > is running, so again not running guest under perf" does not make sense).
> > So, in short, you do not really know where the slow down is coming
> > from.
> 
> That's true.
> 
Then dig dipper.

> > My guess is that it comes from the fact that we unconditionally
> > call clear_atomic_switch_msr() in atomic_switch_perf_msrs(), but then
> > fix that instead of reverting the patch.
> 
> We can try, but reverting is much simpler, it removes code instead of
> adding code. 
Well, this is such absurd statement I do not really know what to say :)

>               Do you know which workload is actually improved by
> 8bf00a529967dafbbb210?
> 
Switching PERF_GLOBAL_CTRL when it needs to be switched.

--
			Gleb.