perf: Default PMU ops
in [Kernel]
|
Prev: Tracing of power:power_start events doesn't work
Next: futex: futex_find_get_task make credentials check conditional
From: Frederic Weisbecker on 29 Jun 2010 10:50 On Thu, Jun 24, 2010 at 04:28:13PM +0200, Peter Zijlstra wrote: > Provide default implementations for the pmu txn methods, this allows > us to remove some conditional code. > > Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > --- > kernel/perf_event.c | 48 ++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > Index: linux-2.6/kernel/perf_event.c > =================================================================== > --- linux-2.6.orig/kernel/perf_event.c > +++ linux-2.6/kernel/perf_event.c > @@ -656,21 +656,14 @@ group_sched_in(struct perf_event *group_ > { > struct perf_event *event, *partial_group = NULL; > struct pmu *pmu = group_event->pmu; > - bool txn = false; > > if (group_event->state == PERF_EVENT_STATE_OFF) > return 0; > > - /* Check if group transaction availabe */ > - if (pmu->start_txn) > - txn = true; > - > - if (txn) > - pmu->start_txn(pmu); > + pmu->start_txn(pmu); > > if (event_sched_in(group_event, cpuctx, ctx)) { > - if (txn) > - pmu->cancel_txn(pmu); > + pmu->cancel_txn(pmu); > return -EAGAIN; > } > > @@ -684,7 +677,7 @@ group_sched_in(struct perf_event *group_ > } > } > > - if (!txn || !pmu->commit_txn(pmu)) > + if (!pmu->commit_txn(pmu)) > return 0; > > group_error: > @@ -699,8 +692,7 @@ group_error: > } > event_sched_out(group_event, cpuctx, ctx); > > - if (txn) > - pmu->cancel_txn(pmu); > + pmu->cancel_txn(pmu); > > return -EAGAIN; > } > @@ -4755,6 +4747,26 @@ static struct list_head pmus; > static DEFINE_MUTEX(pmus_lock); > static struct srcu_struct pmus_srcu; > > +static void perf_pmu_nop(struct pmu *pmu) > +{ > +} > + > +static void perf_pmu_start_txn(struct pmu *pmu) > +{ > + perf_pmu_disable(pmu); > +} > + > +static int perf_pmu_commit_txn(struct pmu *pmu) > +{ > + perf_pmu_enable(pmu); > + return 0; > +} > + > +static void perf_pmu_cancel_txn(struct pmu *pmu) > +{ > + perf_pmu_enable(pmu); Did you mean disable here? -- 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:00 On Thu, Jun 24, 2010 at 04:28:13PM +0200, Peter Zijlstra wrote: > Provide default implementations for the pmu txn methods, this allows > us to remove some conditional code. > > Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > --- > kernel/perf_event.c | 48 ++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > Index: linux-2.6/kernel/perf_event.c > =================================================================== > --- linux-2.6.orig/kernel/perf_event.c > +++ linux-2.6/kernel/perf_event.c > @@ -656,21 +656,14 @@ group_sched_in(struct perf_event *group_ > { > struct perf_event *event, *partial_group = NULL; > struct pmu *pmu = group_event->pmu; > - bool txn = false; > > if (group_event->state == PERF_EVENT_STATE_OFF) > return 0; > > - /* Check if group transaction availabe */ > - if (pmu->start_txn) > - txn = true; > - > - if (txn) > - pmu->start_txn(pmu); > + pmu->start_txn(pmu); > > if (event_sched_in(group_event, cpuctx, ctx)) { > - if (txn) > - pmu->cancel_txn(pmu); > + pmu->cancel_txn(pmu); > return -EAGAIN; > } > > @@ -684,7 +677,7 @@ group_sched_in(struct perf_event *group_ > } > } > > - if (!txn || !pmu->commit_txn(pmu)) > + if (!pmu->commit_txn(pmu)) > return 0; > > group_error: > @@ -699,8 +692,7 @@ group_error: > } > event_sched_out(group_event, cpuctx, ctx); > > - if (txn) > - pmu->cancel_txn(pmu); > + pmu->cancel_txn(pmu); > > return -EAGAIN; > } > @@ -4755,6 +4747,26 @@ static struct list_head pmus; > static DEFINE_MUTEX(pmus_lock); > static struct srcu_struct pmus_srcu; > > +static void perf_pmu_nop(struct pmu *pmu) > +{ > +} > + > +static void perf_pmu_start_txn(struct pmu *pmu) > +{ > + perf_pmu_disable(pmu); > +} > + > +static int perf_pmu_commit_txn(struct pmu *pmu) > +{ > + perf_pmu_enable(pmu); > + return 0; > +} > + > +static void perf_pmu_cancel_txn(struct pmu *pmu) > +{ > + perf_pmu_enable(pmu); > +} So why do you need perf_pmu_*able wrappers now that you brings stubs if none is provided? Actually, one problem is that it makes calling two indirect nops for software events. Should the txn things really map to the enable/disable ops is the off-case? Probably better let pmu implementations deal with that. If they didn't provide txn implementations, it means they don't need it, hence it should directly map to a nop. -- 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 29 Jun 2010 11:00 On Tue, 2010-06-29 at 16:49 +0200, Frederic Weisbecker wrote: > > +static void perf_pmu_start_txn(struct pmu *pmu) > > +{ > > + perf_pmu_disable(pmu); > > +} > > + > > +static int perf_pmu_commit_txn(struct pmu *pmu) > > +{ > > + perf_pmu_enable(pmu); > > + return 0; > > +} > > + > > +static void perf_pmu_cancel_txn(struct pmu *pmu) > > +{ > > + perf_pmu_enable(pmu); > > > Did you mean disable here? Nope, start_txn() will disable the pmu, both commit_txn and cancel_txn can be used to close the transaction and should thus enable the pmu again. -- 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:10 On Tue, Jun 29, 2010 at 04:57:53PM +0200, Peter Zijlstra wrote: > On Tue, 2010-06-29 at 16:49 +0200, Frederic Weisbecker wrote: > > > +static void perf_pmu_start_txn(struct pmu *pmu) > > > +{ > > > + perf_pmu_disable(pmu); > > > +} > > > + > > > +static int perf_pmu_commit_txn(struct pmu *pmu) > > > +{ > > > + perf_pmu_enable(pmu); > > > + return 0; > > > +} > > > + > > > +static void perf_pmu_cancel_txn(struct pmu *pmu) > > > +{ > > > + perf_pmu_enable(pmu); > > > > > > Did you mean disable here? > > Nope, start_txn() will disable the pmu, both commit_txn and cancel_txn > can be used to close the transaction and should thus enable the pmu > again. Looks like I read that while walking on my arms, sorry. -- 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 29 Jun 2010 11:10
On Tue, 2010-06-29 at 16:58 +0200, Frederic Weisbecker wrote: > On Thu, Jun 24, 2010 at 04:28:13PM +0200, Peter Zijlstra wrote: > > Provide default implementations for the pmu txn methods, this allows > > us to remove some conditional code. > > > > Signed-off-by: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > > --- > > kernel/perf_event.c | 48 ++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 36 insertions(+), 12 deletions(-) > > > > Index: linux-2.6/kernel/perf_event.c > > =================================================================== > > --- linux-2.6.orig/kernel/perf_event.c > > +++ linux-2.6/kernel/perf_event.c > > @@ -656,21 +656,14 @@ group_sched_in(struct perf_event *group_ > > { > > struct perf_event *event, *partial_group = NULL; > > struct pmu *pmu = group_event->pmu; > > - bool txn = false; > > > > if (group_event->state == PERF_EVENT_STATE_OFF) > > return 0; > > > > - /* Check if group transaction availabe */ > > - if (pmu->start_txn) > > - txn = true; > > - > > - if (txn) > > - pmu->start_txn(pmu); > > + pmu->start_txn(pmu); > > > > if (event_sched_in(group_event, cpuctx, ctx)) { > > - if (txn) > > - pmu->cancel_txn(pmu); > > + pmu->cancel_txn(pmu); > > return -EAGAIN; > > } > > > > @@ -684,7 +677,7 @@ group_sched_in(struct perf_event *group_ > > } > > } > > > > - if (!txn || !pmu->commit_txn(pmu)) > > + if (!pmu->commit_txn(pmu)) > > return 0; > > > > group_error: > > @@ -699,8 +692,7 @@ group_error: > > } > > event_sched_out(group_event, cpuctx, ctx); > > > > - if (txn) > > - pmu->cancel_txn(pmu); > > + pmu->cancel_txn(pmu); > > > > return -EAGAIN; > > } > > @@ -4755,6 +4747,26 @@ static struct list_head pmus; > > static DEFINE_MUTEX(pmus_lock); > > static struct srcu_struct pmus_srcu; > > > > +static void perf_pmu_nop(struct pmu *pmu) > > +{ > > +} > > + > > +static void perf_pmu_start_txn(struct pmu *pmu) > > +{ > > + perf_pmu_disable(pmu); > > +} > > + > > +static int perf_pmu_commit_txn(struct pmu *pmu) > > +{ > > + perf_pmu_enable(pmu); > > + return 0; > > +} > > + > > +static void perf_pmu_cancel_txn(struct pmu *pmu) > > +{ > > + perf_pmu_enable(pmu); > > +} > > > So why do you need perf_pmu_*able wrappers now that you brings stubs > if none is provided? > > Actually, one problem is that it makes calling two indirect nops > for software events. > > Should the txn things really map to the enable/disable ops is the > off-case? Probably better let pmu implementations deal with that. > If they didn't provide txn implementations, it means they don't need it, > hence it should directly map to a nop. > You mean, if (!pmu->start_txn && pmu->pmu_enable) { /* install defaults */ } ? -- 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/ |