Messages in this thread Patch in this message | | | Date | Mon, 20 Jan 2020 11:50:08 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH] perf/x86/intel/ds: Fix x86_pmu_stop warning for large PEBS |
| |
On Mon, Jan 13, 2020 at 06:09:35AM -0800, kan.liang@linux.intel.com wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > A warning as below may be triggered when sampling large PEBS.
> [ 410.729822] WARNING: CPU: 0 PID: 16397 at arch/x86/events/core.c:1422 > x86_pmu_stop+0x95/0xa0
> For large PEBS, the PEBS buffer can be drained from either NMI handler > or !NMI e.g. context switch. Current implementation doesn't handle them > differently. For !nmi, perf also call the generic overflow handler for > the last PEBS record. That may trigger the interrupt throttle, and stop > the event. That's wrong. > > Here is an example for !NMI scenario, context switch. > Let's say the max_samples_per_tick is adjusted to 2 for some reason. > A context switch happens right after a NMI. > When an old task is scheduled out, it will drain the PEBS buffer, and > then delete the event. > When draining the PEBS buffer, perf_event_overflow() will be called for > the last PEBS record. Since the max_samples_per_tick is only 2, the > interrupt throttle must be triggered. The event will be stopped. > After the draining, the scheduler will delete the event, which stops the > event again. The warning is triggered. > > Perf should handle the NMI and !NMI differently for large PEBS. > For NMI, the generic overflow handler is required for the last PEBS > record. > But, for !NMI, there is no overflow. The generic overflow handler should > not be invoked. Perf should treat the last record exactly the same as > the rest of PEBS records.
Hurmph. there's something there, but the above is hard to read.
drain_pebs() is called from:
- handle_pmi_common() -- sample context - intel_pmu_pebs_sched_task() -- non sample context - intel_pmu_pebs_disable() -- non sample context - intel_pmu_auto_reload_read() -- possible sample context
So the question is what to do for PERF_SAMPLE_READ + PERF_FORMAT_GROUP.
I don't think throttling there is right either, but that does mean the simple in_nmi() test you use is wrong.
Perhaps we can do something with how intel_pmu_drain_pebs_buffer() passes in dummy regs pointer to distinguish between the sample and non sample context.
> --- > arch/x86/events/intel/ds.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c > index 7c896d7e8b6c..51baff083938 100644 > --- a/arch/x86/events/intel/ds.c > +++ b/arch/x86/events/intel/ds.c > @@ -1780,15 +1780,22 @@ static void __intel_pmu_pebs_event(struct perf_event *event, > > setup_sample(event, iregs, at, &data, regs); > > - /* > - * All but the last records are processed. > - * The last one is left to be able to call the overflow handler. > - */ > - if (perf_event_overflow(event, &data, regs)) { > - x86_pmu_stop(event, 0); > - return; > + if (in_nmi()) { > + /* > + * All but the last records are processed. > + * The last one is left to be able to call the overflow handler. > + */ > + if (perf_event_overflow(event, &data, regs)) > + x86_pmu_stop(event, 0); > + } else { > + /* > + * For !NMI, e.g context switch, there is no overflow. > + * The generic overflow handler should not be invoked. > + * Perf should treat the last record exactly the same as the > + * rest of PEBS records. > + */ > + perf_event_output(event, &data, regs); > }
Maybe write it like so?
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c index 4b94ae4ae369..b66be085c7a4 100644 --- a/arch/x86/events/intel/ds.c +++ b/arch/x86/events/intel/ds.c @@ -1747,25 +1747,22 @@ static void __intel_pmu_pebs_event(struct perf_event *event, } else if (!intel_pmu_save_and_restart(event)) return; - while (count > 1) { + while (count > /* cond */) { setup_sample(event, iregs, at, &data, regs); perf_event_output(event, &data, regs); at += cpuc->pebs_record_size; at = get_next_pebs_record_by_bit(at, top, bit); - count--; + if (!--count) + return; } - setup_sample(event, iregs, at, &data, regs); - /* * All but the last records are processed. * The last one is left to be able to call the overflow handler. */ - if (perf_event_overflow(event, &data, regs)) { + setup_sample(event, iregs, at, &data, regs); + if (perf_event_overflow(event, &data, regs)) x86_pmu_stop(event, 0); - return; - } - } static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)
| |