Prev: perf: installed python -report scripts have bogus paths
Next: [PATCH] perf: Makefile fix for Perl scripting
From: Frederic Weisbecker on 18 Jun 2010 00:30
On Wed, Jun 16, 2010 at 06:00:35PM +0200, Peter Zijlstra wrote:
> -static void x86_pmu_stop(struct perf_event *event)
> +static void x86_pmu_stop(struct perf_event *event, int flags)
> {
> - struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> - struct hw_perf_event *hwc = &event->hw;
> - int idx = hwc->idx;
> -
> if (!__test_and_clear_bit(idx, cpuc->active_mask))
> - return;



Do you still need active_mask now that you have HES_STOPPED?



> @@ -4069,6 +4051,9 @@ static int perf_swevent_match(struct per
> struct perf_sample_data *data,
> struct pt_regs *regs)
> {
> + if (event->hw.state)
> + return 0;
> +
> if (event->attr.type != type)
> return 0;
>
> @@ -4211,7 +4196,7 @@ static void perf_swevent_read(struct per
> {
> }
>
> -static int perf_swevent_enable(struct perf_event *event)
> +static int perf_swevent_add(struct perf_event *event, int flags)
> {
> struct hw_perf_event *hwc = &event->hw;
> struct perf_cpu_context *cpuctx;
> @@ -4224,6 +4209,8 @@ static int perf_swevent_enable(struct pe
> perf_swevent_set_period(event);
> }
>
> + hwc->state = !(flags & PERF_EF_START);
> +
> head = find_swevent_head(cpuctx, event);
> if (WARN_ON_ONCE(!head))
> return -EINVAL;
> @@ -4233,13 +4220,19 @@ static int perf_swevent_enable(struct pe
> return 0;
> }
>
> -static void perf_swevent_disable(struct perf_event *event)
> +static void perf_swevent_del(struct perf_event *event, int flags)
> {
> hlist_del_rcu(&event->hlist_entry);
> }
>
> -static void perf_swevent_void(struct perf_event *event)
> +static void perf_swevent_start(struct perf_event *event, int flags)
> +{
> + event->hw.state = 0;
> +}
> +
> +static void perf_swevent_stop(struct perf_event *event, int flags)
> {
> + event->hw.state = 1;
> }


So, instead of doing this and add yet another check in the fast path,
what about just playing with the hlist insertion and deletion?

And if we have PERF_EF_RELOAD in start or PERF_EF_UPDATE in stop,
we simply do nothing, as we know it's just about updating the counter
or reset the interrupt, things that software events just don't care.

And in ->add, you can also do nothing if PERF_EF_START.

It would be nice to have a PERF_EF_STOP as well in ->del, so that
each pmu don't need to maintain an internal state.
If we assume the generic code will never imbalance add/start/stop/del,
or call start after add(PERF_EF_START), or things like this, pmus
like this don't need to ever touch event->hw.state. It's only
necessary for hardware events that call their start/stop internally.


> static inline void perf_tp_register(void)
> @@ -4546,7 +4537,7 @@ void perf_bp_event(struct perf_event *bp
>
> perf_sample_data_init(&sample, bp->attr.bp_addr);
>
> - if (!perf_exclude_event(bp, regs))
> + if (!bp->hw.state && !perf_exclude_event(bp, regs))
> perf_swevent_add(bp, 1, 1, &sample, regs);



Same thing here, and same for trace events.



> }
> #endif
> @@ -4591,12 +4582,12 @@ static void perf_swevent_start_hrtimer(s
> if (hwc->sample_period) {
> u64 period;
>
> - if (hwc->remaining) {
> - if (hwc->remaining < 0)
> + if (hwc->period_left) {
> + if (hwc->period_left < 0)
> period = 10000;
> else
> - period = hwc->remaining;
> - hwc->remaining = 0;
> + period = hwc->period_left;
> + hwc->period_left = 0;



If remaining can be replaced by period_left, it should probably be done
in another patch.

--
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 Jun 2010 03:20
On Fri, 2010-06-18 at 06:21 +0200, Frederic Weisbecker wrote:
> On Wed, Jun 16, 2010 at 06:00:35PM +0200, Peter Zijlstra wrote:
> > -static void x86_pmu_stop(struct perf_event *event)
> > +static void x86_pmu_stop(struct perf_event *event, int flags)
> > {
> > - struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> > - struct hw_perf_event *hwc = &event->hw;
> > - int idx = hwc->idx;
> > -
> > if (!__test_and_clear_bit(idx, cpuc->active_mask))
> > - return;
>
>
>
> Do you still need active_mask now that you have HES_STOPPED?

there still were some users, but yeah, we cuold probably clean that up,
bit since the patch is large enough as is, I didn't attempt that.

> > +static void perf_swevent_start(struct perf_event *event, int flags)
> > +{
> > + event->hw.state = 0;
> > +}
> > +
> > +static void perf_swevent_stop(struct perf_event *event, int flags)
> > {
> > + event->hw.state = 1;
> > }
>
>
> So, instead of doing this and add yet another check in the fast path,
> what about just playing with the hlist insertion and deletion?

I wanted to avoid too much trickery, first make a simple one work, then
try something fancy.

> It would be nice to have a PERF_EF_STOP as well in ->del, so that
> each pmu don't need to maintain an internal state.

You have to track it since we can stop the thing outselves without the
caller knowing.

> > }
> > #endif
> > @@ -4591,12 +4582,12 @@ static void perf_swevent_start_hrtimer(s
> > if (hwc->sample_period) {
> > u64 period;
> >
> > - if (hwc->remaining) {
> > - if (hwc->remaining < 0)
> > + if (hwc->period_left) {
> > + if (hwc->period_left < 0)
> > period = 10000;
> > else
> > - period = hwc->remaining;
> > - hwc->remaining = 0;
> > + period = hwc->period_left;
> > + hwc->period_left = 0;
>
>
>
> If remaining can be replaced by period_left, it should probably be done
> in another patch.

true.
--
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 22 Jun 2010 12:30
On Fri, Jun 18, 2010 at 09:15:38AM +0200, Peter Zijlstra wrote:
> On Fri, 2010-06-18 at 06:21 +0200, Frederic Weisbecker wrote:
> > On Wed, Jun 16, 2010 at 06:00:35PM +0200, Peter Zijlstra wrote:
> > > -static void x86_pmu_stop(struct perf_event *event)
> > > +static void x86_pmu_stop(struct perf_event *event, int flags)
> > > {
> > > - struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> > > - struct hw_perf_event *hwc = &event->hw;
> > > - int idx = hwc->idx;
> > > -
> > > if (!__test_and_clear_bit(idx, cpuc->active_mask))
> > > - return;
> >
> >
> >
> > Do you still need active_mask now that you have HES_STOPPED?
>
> there still were some users, but yeah, we cuold probably clean that up,
> bit since the patch is large enough as is, I didn't attempt that.


Yeah, that can be done later.



> > > +static void perf_swevent_start(struct perf_event *event, int flags)
> > > +{
> > > + event->hw.state = 0;
> > > +}
> > > +
> > > +static void perf_swevent_stop(struct perf_event *event, int flags)
> > > {
> > > + event->hw.state = 1;
> > > }
> >
> >
> > So, instead of doing this and add yet another check in the fast path,
> > what about just playing with the hlist insertion and deletion?
>
> I wanted to avoid too much trickery, first make a simple one work, then
> try something fancy.



Ok, as far it's not considered a long term thing. I can
improve that from my exclusion patchset, rebased on top of yours.




> > It would be nice to have a PERF_EF_STOP as well in ->del, so that
> > each pmu don't need to maintain an internal state.
>
> You have to track it since we can stop the thing outselves without the
> caller knowing.


From the pmu internals yeah, that's what the x86 pmu does. But otherwise, other
pmu don't do such things.

--
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 23 Jun 2010 16:50
On Tue, 2010-06-22 at 18:26 +0200, Frederic Weisbecker wrote:
> > > It would be nice to have a PERF_EF_STOP as well in ->del, so that
> > > each pmu don't need to maintain an internal state.
> >
> > You have to track it since we can stop the thing outselves without the
> > caller knowing.
>
>
> From the pmu internals yeah, that's what the x86 pmu does. But otherwise, other
> pmu don't do such things.

Everybody who does throttling will have to.
--
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 29 Jun 2010 11:40
On Thu, Jun 24, 2010 at 04:28:15PM +0200, Peter Zijlstra wrote:
> Replace pmu::{enable,disable,start,stop,unthrottle} with
> pmu::{add,del,start,stop}, all of which take a flags argument.
>
> The new interface extends the capability to stop a counter while
> keeping it scheduled on the PMU. We replace the throttled state with
> the generic stopped state.
>
> This also allows us to efficiently stop/start counters over certain
> code paths (like IRQ handlers).
>
> It also allows scheduling a counter without it starting, allowing for
> a generic frozen state (useful for rotating stopped counters).
>
> The stopped state is implemented in two different ways, depending on
> how the architecture implemented the throttled state:
>
> 1) We disable the counter:
> a) the pmu has per-counter enable bits, we flip that
> b) we program a NOP event, preserving the counter state
>
> 2) We store the counter state and ignore all read/overflow events
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl>


Acked-by: Frederic Weisbecker <fweisbec(a)gmail.com>

--
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/
 |  Next  |  Last
Pages: 1 2
Prev: perf: installed python -report scripts have bogus paths
Next: [PATCH] perf: Makefile fix for Perl scripting