From: Frederic Weisbecker on
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
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
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
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
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/