Re: [PATCH v5 1/8] perf: Add a flags parameter to pmu txn interfaces
From: Peter Zijlstra <peterz@infradead.org>
Date: 2015-09-01 10:39:10
Also in:
linux-s390, lkml, sparclinux
On Thu, Aug 13, 2015 at 11:49:34PM -0700, Sukadev Bhattiprolu wrote: I'm ever so sorry I keep going on about this, but..
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index d90893b..b18efe4 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c@@ -50,6 +50,7 @@ struct cpu_hw_events { unsigned int group_flag; int n_txn_start; + int txn_flags; /* BHRB bits */ u64 bhrb_filter; /* BHRB HW branch filter */@@ -1586,11 +1587,19 @@ static void power_pmu_stop(struct perf_event *event, int ef_flags) * Start group events scheduling transaction * Set the flag to make pmu::enable() not perform the * schedulability test, it will be performed at commit time + * + * We only support PERF_PMU_TXN_ADD transactions. Save the + * transaction flags but otherwise ignore non-PERF_PMU_TXN_ADD + * transactions. */ -static void power_pmu_start_txn(struct pmu *pmu) +static void power_pmu_start_txn(struct pmu *pmu, int txn_flags) { struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events); + cpuhw->txn_flags = txn_flags; + if (txn_flags & ~PERF_PMU_TXN_ADD) + return; + perf_pmu_disable(pmu); cpuhw->group_flag |= PERF_EVENT_TXN; cpuhw->n_txn_start = cpuhw->n_events;@@ -1604,6 +1613,12 @@ static void power_pmu_start_txn(struct pmu *pmu) static void power_pmu_cancel_txn(struct pmu *pmu) { struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events); + int txn_flags; + + txn_flags = cpuhw->txn_flags; + cpuhw->txn_flags = 0; + if (txn_flags & ~PERF_PMU_TXN_ADD) + return; cpuhw->group_flag &= ~PERF_EVENT_TXN; perf_pmu_enable(pmu);@@ -1618,10 +1633,18 @@ static int power_pmu_commit_txn(struct pmu *pmu) { struct cpu_hw_events *cpuhw; long i, n; + int txn_flags; if (!ppmu) return -EAGAIN; + cpuhw = this_cpu_ptr(&cpu_hw_events); + + txn_flags = cpuhw->txn_flags; + cpuhw->txn_flags = 0; + if (cpuhw->txn_flags & ~PERF_PMU_TXN_ADD) + return 0; + n = cpuhw->n_events; if (check_excludes(cpuhw->event, cpuhw->flags, 0, n)) return -EAGAIN;
when looking at this (I almost pressed A for apply) it occurred to me that we now keep double state, cpuhw->txn_flags and cpuhw->group_flag are basically the same thing. Would not something like the below avoid this duplication?
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c@@ -48,9 +48,8 @@ struct cpu_hw_events { unsigned long amasks[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES]; unsigned long avalues[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES]; - unsigned int group_flag; + unsigned int txn_flags; int n_txn_start; - int txn_flags; /* BHRB bits */ u64 bhrb_filter; /* BHRB HW branch filter */
@@ -1442,7 +1441,7 @@ static int power_pmu_add(struct perf_eve * skip the schedulability test here, it will be performed * at commit time(->commit_txn) as a whole */ - if (cpuhw->group_flag & PERF_EVENT_TXN) + if (cpuhw->txn_flags & PERF_PMU_TXN_ADD) goto nocheck; if (check_excludes(cpuhw->event, cpuhw->flags, n0, 1))
@@ -1596,12 +1595,12 @@ static void power_pmu_start_txn(struct p { struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events); + WARN_ON_ONCE(cpuhw->txn_flags); /* already txn in flight */ cpuhw->txn_flags = txn_flags; if (txn_flags & ~PERF_PMU_TXN_ADD) return; perf_pmu_disable(pmu); - cpuhw->group_flag |= PERF_EVENT_TXN; cpuhw->n_txn_start = cpuhw->n_events; }
@@ -1615,12 +1614,12 @@ static void power_pmu_cancel_txn(struct struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events); int txn_flags; + WARN_ON_ONCE(!cpuhw->txn_flags); /* no txn in flight */ txn_flags = cpuhw->txn_flags; cpuhw->txn_flags = 0; if (txn_flags & ~PERF_PMU_TXN_ADD) return; - cpuhw->group_flag &= ~PERF_EVENT_TXN; perf_pmu_enable(pmu); }
@@ -1633,17 +1632,17 @@ static int power_pmu_commit_txn(struct p { struct cpu_hw_events *cpuhw; long i, n; - int txn_flags; if (!ppmu) return -EAGAIN; cpuhw = this_cpu_ptr(&cpu_hw_events); - txn_flags = cpuhw->txn_flags; - cpuhw->txn_flags = 0; - if (cpuhw->txn_flags & ~PERF_PMU_TXN_ADD) + WARN_ON_ONCE(!cpuhw->txn_flags); /* no txn in flight */ + if (cpuhw->txn_flags & ~PERF_PMU_TXN_ADD) { + cpuhw->txn_flags = 0; return 0; + } n = cpuhw->n_events; if (check_excludes(cpuhw->event, cpuhw->flags, 0, n))
@@ -1655,7 +1654,7 @@ static int power_pmu_commit_txn(struct p for (i = cpuhw->n_txn_start; i < n; ++i) cpuhw->event[i]->hw.config = cpuhw->events[i]; - cpuhw->group_flag &= ~PERF_EVENT_TXN; + cpuhw->txn_flags = 0; perf_pmu_enable(pmu); return 0; } --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h
@@ -199,9 +199,7 @@ struct perf_event; /* * Common implementation detail of pmu::{start,commit,cancel}_txn */ -#define PERF_EVENT_TXN 0x1 - -#define PERF_PMU_TXN_ADD 0x1 /* txn to add/schedule event on PMU */ +#define PERF_PMU_TXN_ADD 0x1 /* txn to add/schedule event on PMU */ /** * pmu::capabilities flags