Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/5] perf: Provide a proper stop action for software events | From | Peter Zijlstra <> | Date | Thu, 10 Jun 2010 12:46:53 +0200 |
| |
On Thu, 2010-06-10 at 05:49 +0200, Frederic Weisbecker wrote: > In order to introduce new context exclusions, software events will > have to eventually stop when needed. We'll want perf_event_stop() to > act on every events. > > To achieve this, remove the stub stop/start pmu callbacks of software > and tracepoint events. > > This may even optimize the case of hardware and software events > running at the same time: now we only stop/start all hardware > events if we reset a hardware event period, not anymore with > software events. > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Stephane Eranian <eranian@google.com> > Cc: Cyrill Gorcunov <gorcunov@gmail.com> > Cc: Zhang Yanmin <yanmin_zhang@linux.intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > --- > kernel/perf_event.c | 29 ++++++++++++++++------------- > 1 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/kernel/perf_event.c b/kernel/perf_event.c > index c772a3d..5c004f7 100644 > --- a/kernel/perf_event.c > +++ b/kernel/perf_event.c > @@ -1541,11 +1541,23 @@ static void perf_adjust_period(struct perf_event *event, u64 nsec, u64 count) > hwc->sample_period = sample_period; > > if (local64_read(&hwc->period_left) > 8*sample_period) { > - perf_disable(); > - perf_event_stop(event); > + bool software_event = is_software_event(event); > + > + /* > + * Only hardware events need their irq period to be > + * reprogrammed > + */ > + if (!software_event) { > + perf_disable(); > + perf_event_stop(event); > + } > + > local64_set(&hwc->period_left, 0); > - perf_event_start(event); > - perf_enable(); > + > + if (!software_event) { > + perf_event_start(event); > + perf_enable(); > + } > } > } > > @@ -4286,16 +4298,9 @@ static void perf_swevent_void(struct perf_event *event) > { > } > > -static int perf_swevent_int(struct perf_event *event) > -{ > - return 0; > -} > - > static const struct pmu perf_ops_generic = { > .enable = perf_swevent_enable, > .disable = perf_swevent_disable, > - .start = perf_swevent_int, > - .stop = perf_swevent_void, > .read = perf_swevent_read, > .unthrottle = perf_swevent_void, /* hwc->interrupts already reset */ > }; > @@ -4578,8 +4583,6 @@ static int swevent_hlist_get(struct perf_event *event) > static const struct pmu perf_ops_tracepoint = { > .enable = perf_trace_enable, > .disable = perf_trace_disable, > - .start = perf_swevent_int, > - .stop = perf_swevent_void, > .read = perf_swevent_read, > .unthrottle = perf_swevent_void, > };
I really don't like this.. we should be removing differences between software and hardware pmu implementations, not add more :/
Something like the below would work, the only 'problem' is that it grows hw_perf_event.
--- include/linux/perf_event.h | 1 + kernel/perf_event.c | 27 ++++++++++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 9073bde..2292659 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -531,6 +531,7 @@ struct hw_perf_event { struct { /* software */ s64 remaining; struct hrtimer hrtimer; + int stopped; }; #ifdef CONFIG_HAVE_HW_BREAKPOINT /* breakpoint */ diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 403d180..14b691e 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -4113,6 +4113,9 @@ static int perf_swevent_match(struct perf_event *event, struct perf_sample_data *data, struct pt_regs *regs) { + if (event->hw.stopped) + return 0; + if (event->attr.type != type) return 0; @@ -4282,22 +4285,28 @@ static void perf_swevent_disable(struct perf_event *event) hlist_del_rcu(&event->hlist_entry); } -static void perf_swevent_void(struct perf_event *event) +static void perf_swevent_throttle(struct perf_event *event) { + /* hwc->interrupts already reset */ } -static int perf_swevent_int(struct perf_event *event) +static int perf_swevent_start(struct perf_event *event) { - return 0; + event->hw.stopped = 0; +} + +static void perf_swevent_throttle(struct perf_event *event) +{ + event->hw.stopped = 1; } static const struct pmu perf_ops_generic = { .enable = perf_swevent_enable, .disable = perf_swevent_disable, - .start = perf_swevent_int, - .stop = perf_swevent_void, + .start = perf_swevent_start, + .stop = perf_swevent_stop, .read = perf_swevent_read, - .unthrottle = perf_swevent_void, /* hwc->interrupts already reset */ + .unthrottle = perf_swevent_throttle, }; /* @@ -4578,10 +4587,10 @@ static int swevent_hlist_get(struct perf_event *event) static const struct pmu perf_ops_tracepoint = { .enable = perf_trace_enable, .disable = perf_trace_disable, - .start = perf_swevent_int, - .stop = perf_swevent_void, + .start = perf_swevent_start, + .stop = perf_swevent_stop, .read = perf_swevent_read, - .unthrottle = perf_swevent_void, + .unthrottle = perf_swevent_throttle, }; static int perf_tp_filter_match(struct perf_event *event,
| |