Re: [V2 PATCH 0/6] x86, NMI: give NMI handler a face-lift

[Don Zickus <dzickus@redhat.com>]

On Thu, Nov 18, 2010 at 11:39:48PM +0300, Cyrill Gorcunov wrote:
> On Thu, Nov 18, 2010 at 11:28:50PM +0300, Cyrill Gorcunov wrote:
> > On Thu, Nov 18, 2010 at 03:08:07PM -0500, Don Zickus wrote:
> > > On Thu, Nov 18, 2010 at 01:51:44PM -0600, Jason Wessel wrote:
> > > > > So the problem is when the nmi watchdog is enabled, the perf event is
> > > > > 'active' and thus tries to read the counter value.  Because it is always
> > > > > zero, perf just assumes the counter overflowed and the NMI is his.
> > > > >
> > > > > Not sure how to fix it yet, other than include the logic that detects we
> > > > > are on a guest and disable perf??
> > > > >
> > > > >   
> > > > 
> > > > I highly doubt we want to disable perf.   I would rather use the source
> > > > and fix the nmi emulation in KVM/Qemu after we hear back the results
> > > 
> > > Well I think Peter does not have a positive opinion about emulating perf
> > > inside a guest.  Nor are the KVM folks having much success in doing so.
> > > 
> > > Just to clarify, perf counter emulation is _not_ implemented in kvm.
> > > Therefore disabling perf in the guest makes sense until someone gets
> > > around to actually writing the emulation code for perf in a guest. :-)
> > > 
> > > Cheers,
> > > Don
> > 
> >  Don, Robert,
> > 
> >  I still have suspicious on ours 'pending' nmi handler. Look what I mean --
> > (keep in mind that p4 has a a way more counters than others).
> > 
> 
>  To be precise -- it seems this scenario may force the back-to-back
> nmi handler to eat unknown nmi.

That was the point of the change to do exactly that.

The problem is/was when you go to check to see if the period expired in
x86_perf_event_set_period(), you refresh the perf counter.  The next step
is to see if the event period has expired, if so disabled the 'active'
bit.

However, there is a race between when you refresh the counter to when you
actually disable it, such that you may cause the counter to overflow again
and thus generate another NMI.  The whole ->running thing was implemented
by Robert to try and check for that condition and eat the NMI as we have
no intention of handling it (because it is bogus).

The alternative is to use another rdmsrl to actually see if we trigger
another NMI.  This was deemed a performance hit for such a small case.

Cheers,
Don