[PATCH] perf: fix the is_software_event() definition
in [Kernel]
|
Prev: (none)
Next: leds: make PCI device id constant
From: Peter Zijlstra on 18 Jan 2010 07:00 On Mon, 2010-01-18 at 12:13 +0100, Peter Zijlstra wrote: > On Sun, 2010-01-17 at 15:12 +0100, Frederic Weisbecker wrote: > > > You need to also call pmu->disable() if it is a software event, > > because a breakpoint needs to be unregistered in hardware level > > too. > > breakpoint isn't a software pmu. But yeah, enable and disable need to > match. That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one. Which seems to suggest the following --- Subject: perf: fix the is_software_event() definition When adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression. Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> --- include/linux/perf_event.h | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index c66b34f..835ba26 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -814,9 +814,13 @@ extern int perf_event_overflow(struct perf_event *event, int nmi, */ static inline int is_software_event(struct perf_event *event) { - return (event->attr.type != PERF_TYPE_RAW) && - (event->attr.type != PERF_TYPE_HARDWARE) && - (event->attr.type != PERF_TYPE_HW_CACHE); + switch (event->attr.type) { + case PERF_TYPE_SOFTWARE: + case PERF_TYPE_TRACEPOINT: + case PERF_TYPE_HW_CACHE: + return 1; + } + return 0; } extern atomic_t perf_swevent_enabled[PERF_COUNT_SW_MAX]; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: stephane eranian on 18 Jan 2010 08:00 On Mon, Jan 18, 2010 at 12:53 PM, Peter Zijlstra <peterz(a)infradead.org> wrote: > On Mon, 2010-01-18 at 12:13 +0100, Peter Zijlstra wrote: >> On Sun, 2010-01-17 at 15:12 +0100, Frederic Weisbecker wrote: >> >> > You need to also call pmu->disable() if it is a software event, >> > because a breakpoint needs to be unregistered in hardware level >> > too. >> >> breakpoint isn't a software pmu. But yeah, enable and disable need to >> match. > > That is, it shouldn't be a software pmu, because we assume software > events can always be scheduled, whereas that's definitely not so for the > breakpoint one. > > Which seems to suggest the following > > --- > Subject: perf: fix the is_software_event() definition > > When adding the breakpoint pmu Frederic forgot to exclude it from being > a software event. While we're at it, make it an inclusive expression. > > Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > --- > include/linux/perf_event.h | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index c66b34f..835ba26 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -814,9 +814,13 @@ extern int perf_event_overflow(struct perf_event *event, int nmi, > */ > static inline int is_software_event(struct perf_event *event) > { > - return (event->attr.type != PERF_TYPE_RAW) && > - (event->attr.type != PERF_TYPE_HARDWARE) && > - (event->attr.type != PERF_TYPE_HW_CACHE); > + switch (event->attr.type) { > + case PERF_TYPE_SOFTWARE: > + case PERF_TYPE_TRACEPOINT: > + case PERF_TYPE_HW_CACHE: > + return 1; > + } > + return 0; > } > > extern atomic_t perf_swevent_enabled[PERF_COUNT_SW_MAX]; > PERF_TYPE_HW_CACHE is a hardware PMU event. > > > ------------------------------------------------------------------------------ > Throughout its 18-year history, RSA Conference consistently attracts the > world's best and brightest in the field, creating opportunities for Conference > attendees to learn about information security's most important issues through > interactions with peers, luminaries and emerging and established companies. > http://p.sf.net/sfu/rsaconf-dev2dev > _______________________________________________ > perfmon2-devel mailing list > perfmon2-devel(a)lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/perfmon2-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: stephane eranian on 18 Jan 2010 08:00 On Mon, Jan 18, 2010 at 1:07 PM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote: > On Mon, Jan 18, 2010 at 12:53:36PM +0100, Peter Zijlstra wrote: >> On Mon, 2010-01-18 at 12:13 +0100, Peter Zijlstra wrote: >> > On Sun, 2010-01-17 at 15:12 +0100, Frederic Weisbecker wrote: >> > >> > > You need to also call pmu->disable() if it is a software event, >> > > because a breakpoint needs to be unregistered in hardware level >> > > too. >> > >> > breakpoint isn't a software pmu. But yeah, enable and disable need to >> > match. >> >> That is, it shouldn't be a software pmu, because we assume software >> events can always be scheduled, whereas that's definitely not so for the >> breakpoint one. >> >> Which seems to suggest the following >> >> --- >> Subject: perf: fix the is_software_event() definition >> >> When adding the breakpoint pmu Frederic forgot to exclude it from being >> a software event. While we're at it, make it an inclusive expression. >> >> Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > > > > Agreed. > > But then Stephane will need to update his patch and use > something else than is_software_event() to guess if an event > needs its pmu->enable/disable to be called. > > A kind of helper that can tell: I am not handled by > hw_perf_group_sched_in() > Then, we should use something that checks if the event is handled by the X86 PMU layer: int is_x86_hw_event(struct perf_event *event) { return event->pmu == x86_pmu; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 18 Jan 2010 08:10 2010/1/18 stephane eranian <eranian(a)googlemail.com>: > On Mon, Jan 18, 2010 at 1:07 PM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote: >> On Mon, Jan 18, 2010 at 12:53:36PM +0100, Peter Zijlstra wrote: >>> On Mon, 2010-01-18 at 12:13 +0100, Peter Zijlstra wrote: >>> > On Sun, 2010-01-17 at 15:12 +0100, Frederic Weisbecker wrote: >>> > >>> > > You need to also call pmu->disable() if it is a software event, >>> > > because a breakpoint needs to be unregistered in hardware level >>> > > too. >>> > >>> > breakpoint isn't a software pmu. But yeah, enable and disable need to >>> > match. >>> >>> That is, it shouldn't be a software pmu, because we assume software >>> events can always be scheduled, whereas that's definitely not so for the >>> breakpoint one. >>> >>> Which seems to suggest the following >>> >>> --- >>> Subject: perf: fix the is_software_event() definition >>> >>> When adding the breakpoint pmu Frederic forgot to exclude it from being >>> a software event. While we're at it, make it an inclusive expression. >>> >>> Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> >> >> >> >> Agreed. >> >> But then Stephane will need to update his patch and use >> something else than is_software_event() to guess if an event >> needs its pmu->enable/disable to be called. >> >> A kind of helper that can tell: I am not handled by >> hw_perf_group_sched_in() >> > Then, we should use something that checks if the event > is handled by the X86 PMU layer: > > int is_x86_hw_event(struct perf_event *event) > { > � return event->pmu == x86_pmu; > } > Yeah. I missed this patch from Peter in its answer. Looks good. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 18 Jan 2010 08:10
On Mon, 2010-01-18 at 13:53 +0100, stephane eranian wrote: > PERF_TYPE_HW_CACHE is a hardware PMU event. D'0h! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |