Re: [V4][PATCH 4/6] x86, nmi: add in logic to handle multipleevents and unknown NMIs
From: Robert Richter
Date: Thu Sep 15 2011 - 12:56:25 EST
On 14.09.11 22:16:12, Robert Richter wrote:
> On 14.09.11 13:58:09, Don Zickus wrote:
> > On Wed, Sep 14, 2011 at 06:26:53PM +0200, Robert Richter wrote:
> > > On 13.09.11 16:58:27, Don Zickus wrote:
> > > > @@ -87,6 +87,16 @@ static int notrace __kprobes nmi_handle(unsigned int type, struct pt_regs *regs)
> > > >
> > > > handled += a->handler(type, regs);
> > > >
> > > > + /*
> > > > + * Optimization: only loop once if this is not a
> > > > + * back-to-back NMI. The idea is nothing is dropped
> > > > + * on the first NMI, only on the second of a back-to-back
> > > > + * NMI. No need to waste cycles going through all the
> > > > + * handlers.
> > > > + */
> > > > + if (!b2b && handled)
> > > > + break;
> > >
> > > Don, if I am not missing something, this actually does not work
> > > because perfctr NMIs do not re-trigger. Suppose a handler running
> > > before perfctr. It sets 'handled' and the chain is stopped here. To
> > > run through the perfctr handler the NMI must retrigger which it
> > > doesn't.
> >
> > Your patch is incorrect. Your dummy handler does not handle a _real_ NMI.
> > Which means no _real_ NMI was ever generated. Of course perf won't work.
> > You just swallowed its NMI.
> >
> > The change I made is for nmi handlers that actually have an NMI associated
> > with them. The idea is if somebody generated an NMI, it will get handled
> > by a handler. If perf comes along and generates another NMI, it should
> > get latched. Upon handling the first NMI, the perf NMI should be sitting
> > queued up and cause the back-to-back NMI. In this case all the handlers
> > will be executed (to handle dropped NMIs).
>
> Yes, your thought about the latched NMI could work. Though I better
> test this with some real nmis from different sources. Unfortunately
> this is much harder to trigger. Will give it a try. It would be a
> pretty nice optimization then.
Don,
I did some tests today with parallel IBS and perfctr sessions running.
I see cases for all combinations of NMI back-to-back sequences (see
below for the traces, the patch and attached log file):
[1] IBS - IBS
[2] IBS - perfctr
[3] perfctr - IBS
[4] perfctr - perfctr
So we see that all cases exists. Unfortunately this is not an evidence
that the approach works in *any* case, because we don't see
potentially lost entries. This is hard to prove. But I think the can
assume it works as expected. I also asked the hw guys for
clarification. Will let you know if we must modify the algorithm.
-Robert
Some back-to-back traces:
<...>-2358 [002] 35.807818: perf_ibs_nmi_handler: b2b: seq 335: handled: 1 (#336), last handled: 1 (#335)
<...>-2358 [002] 35.808396: perf_ibs_nmi_handler: b2b: seq 340: handled: 1 (#341), last handled: 1 (#340) [1]
<...>-2358 [002] 35.814160: perf_ibs_nmi_handler: b2b: seq 391: handled: 1 (#392), last handled: 1 (#391)
<...>-2358 [002] 35.818585: perf_ibs_nmi_handler: b2b: seq 430: handled: 1 (#431), last handled: 1 (#430)
<...>-2349 [007] 36.026940: perf_ibs_nmi_handler: b2b: seq 2338: handled: 1 (#2339), last handled: 1 (#2338)
<...>-2349 [007] 36.027063: perf_ibs_nmi_handler: b2b: seq 2364: handled: 1 (#2365), last handled: 0 (#2364)
<...>-2349 [007] 36.027064: perf_event_nmi_handler: b2b: seq 168: handled: 0 (#2365), last handled: 1 (#2364)
<...>-2349 [007] 36.027066: perf_ibs_nmi_handler: b2b: seq 2365: handled: 0 (#2366), last handled: 1 (#2365) [2]
<...>-2349 [007] 36.027068: perf_event_nmi_handler: b2b: seq 169: handled: 1 (#2366), last handled: 0 (#2365) [2]
<...>-2349 [007] 36.027183: perf_ibs_nmi_handler: b2b: seq 2389: handled: 0 (#2390), last handled: 0 (#2389)
<...>-2349 [007] 36.027185: perf_event_nmi_handler: b2b: seq 193: handled: 1 (#2390), last handled: 1 (#2389)
<...>-2349 [007] 36.027189: perf_ibs_nmi_handler: b2b: seq 2391: handled: 1 (#2392), last handled: 0 (#2391) [3]
<...>-2349 [007] 36.027191: perf_event_nmi_handler: b2b: seq 195: handled: 0 (#2392), last handled: 1 (#2391) [3]
<...>-2349 [007] 36.027193: perf_ibs_nmi_handler: b2b: seq 2392: handled: 0 (#2393), last handled: 1 (#2392)
<...>-2349 [007] 36.027195: perf_event_nmi_handler: b2b: seq 196: handled: 1 (#2393), last handled: 0 (#2392)
<...>-2349 [007] 36.027206: perf_ibs_nmi_handler: b2b: seq 2395: handled: 0 (#2396), last handled: 0 (#2395)
<...>-2349 [007] 36.027208: perf_event_nmi_handler: b2b: seq 199: handled: 1 (#2396), last handled: 1 (#2395)
<...>-2349 [007] 36.027212: perf_ibs_nmi_handler: b2b: seq 2396: handled: 0 (#2397), last handled: 0 (#2396)
<...>-2349 [007] 36.027215: perf_event_nmi_handler: b2b: seq 200: handled: 1 (#2397), last handled: 1 (#2396)
<...>-2349 [007] 36.027218: perf_ibs_nmi_handler: b2b: seq 2397: handled: 0 (#2398), last handled: 0 (#2397)
<...>-2349 [007] 36.027221: perf_event_nmi_handler: b2b: seq 201: handled: 1 (#2398), last handled: 1 (#2397)
<...>-2349 [007] 36.027224: perf_ibs_nmi_handler: b2b: seq 2398: handled: 0 (#2399), last handled: 0 (#2398)
<...>-2349 [007] 36.027227: perf_event_nmi_handler: b2b: seq 202: handled: 1 (#2399), last handled: 1 (#2398)
<...>-2349 [007] 36.027302: perf_ibs_nmi_handler: b2b: seq 2414: handled: 0 (#2415), last handled: 0 (#2414)
<...>-2358 [002] 36.026053: perf_ibs_nmi_handler: b2b: seq 2306: handled: 0 (#2307), last handled: 0 (#2306)
<...>-2358 [002] 36.026056: perf_event_nmi_handler: b2b: seq 115: handled: 1 (#2307), last handled: 1 (#2306)
<...>-2358 [002] 36.026081: perf_ibs_nmi_handler: b2b: seq 2313: handled: 0 (#2314), last handled: 0 (#2313) [4]
<...>-2358 [002] 36.026083: perf_event_nmi_handler: b2b: seq 121: handled: 1 (#2314), last handled: 1 (#2313) [4]
<...>-2358 [002] 36.026115: perf_ibs_nmi_handler: b2b: seq 2320: handled: 0 (#2321), last handled: 0 (#2320)
<...>-2358 [002] 36.026117: perf_event_nmi_handler: b2b: seq 128: handled: 1 (#2321), last handled: 1 (#2320)
>
> > My only question to you is the IBS stuff you were working on. Does that
> > generate a _real_ NMI or does it just piggy back off of the perf NMI?
>
> Yes, IBS generates real NMIs, there is an own interrupt vector for
> it.
>
> -Robert
>
> --
> Advanced Micro Devices, Inc.
> Operating System Research Center