Re: [PATCH 1/4] perf: Add 'flags' parameter to pmu txn interfaces

[Sukadev Bhattiprolu]

Peter Zijlstra [pet...@infradead.org] wrote:
| On Wed, Mar 04, 2015 at 12:35:05AM -0800, Sukadev Bhattiprolu wrote:
|  In addition to using the transaction interface to schedule events
|  on a PMU, we will use it to also read a group of counters at once.
|  Accordingly, add a flags parameter to the transaction interfaces.
|  The flags indicate wheether the transaction is to add events to
|  the PMU (PERF_PMU_TXN_ADD) or to read the events PERF_PMU_TXN_READ.
|  
|  Based on input from Peter Zijlstra.
|  
|  Signed-off-by: Sukadev Bhattiprolu suka...@linux.vnet.ibm.com
|  ---
|   arch/powerpc/perf/core-book3s.c  | 15 ---
|   arch/x86/kernel/cpu/perf_event.c | 15 ---
|   include/linux/perf_event.h   | 14 +++---
|   kernel/events/core.c | 26 +++---
|   4 files changed, 50 insertions(+), 20 deletions(-)
| 
| s390 and sparc also implement the txn.

Yes, I have fixed that now. Was mostly exploring the basic txn interface.
| 
| # git grep \.start_txn
| arch/powerpc/perf/core-book3s.c:.start_txn  = power_pmu_start_txn,
| arch/s390/kernel/perf_cpum_cf.c:.start_txn= cpumf_pmu_start_txn,
| arch/sparc/kernel/perf_event.c: .start_txn  = sparc_pmu_start_txn,
| arch/x86/kernel/cpu/perf_event.c:   .start_txn  = 
x86_pmu_start_txn,
| 
| Also; you add the flag to all 3 calls; does it make sense to only pass
| it to the first and save the txn type in the txn state itself? We could
| add PERF_EVENT_TXN_READ for this..

We could do that. The one small downside I see with passing the txn flag
only ot -start_txn() is that checks like this become more complicated,
even in PMUs that don't care about the TXN_READ transactions.

@@ -1619,8 +1622,11 @@ static void x86_pmu_start_txn(struct pmu *pmu)
  * Clear the flag and pmu::enable() will perform the
  * schedulability test.
  */
-static void x86_pmu_cancel_txn(struct pmu *pmu)
+static void x86_pmu_cancel_txn(struct pmu *pmu, int flags)
 {
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return;
+

The -start_txn will need to save the transaction type in the
architecture's 'cpuhw' and check/clear in -commit_txn() and
-clear_txn() - right ?

Sukadev

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/4] perf: Add 'flags' parameter to pmu txn interfaces

[Sukadev Bhattiprolu]

In addition to using the transaction interface to schedule events
on a PMU, we will use it to also read a group of counters at once.
Accordingly, add a flags parameter to the transaction interfaces.
The flags indicate wheether the transaction is to add events to
the PMU (PERF_PMU_TXN_ADD) or to read the events PERF_PMU_TXN_READ.

Based on input from Peter Zijlstra.

Signed-off-by: Sukadev Bhattiprolu suka...@linux.vnet.ibm.com
---
 arch/powerpc/perf/core-book3s.c  | 15 ---
 arch/x86/kernel/cpu/perf_event.c | 15 ---
 include/linux/perf_event.h   | 14 +++---
 kernel/events/core.c | 26 +++---
 4 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 7c4f669..3d3739a 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1573,10 +1573,13 @@ static void power_pmu_stop(struct perf_event *event, 
int ef_flags)
  * Set the flag to make pmu::enable() not perform the
  * schedulability test, it will be performed at commit time
  */
-static void power_pmu_start_txn(struct pmu *pmu)
+static void power_pmu_start_txn(struct pmu *pmu, int flags)
 {
struct cpu_hw_events *cpuhw = this_cpu_ptr(cpu_hw_events);
 
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return;
+
perf_pmu_disable(pmu);
cpuhw-group_flag |= PERF_EVENT_TXN;
cpuhw-n_txn_start = cpuhw-n_events;
@@ -1587,10 +1590,13 @@ static void power_pmu_start_txn(struct pmu *pmu)
  * Clear the flag and pmu::enable() will perform the
  * schedulability test.
  */
-static void power_pmu_cancel_txn(struct pmu *pmu)
+static void power_pmu_cancel_txn(struct pmu *pmu, int flags)
 {
struct cpu_hw_events *cpuhw = this_cpu_ptr(cpu_hw_events);
 
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return;
+
cpuhw-group_flag = ~PERF_EVENT_TXN;
perf_pmu_enable(pmu);
 }
@@ -1600,11 +1606,14 @@ static void power_pmu_cancel_txn(struct pmu *pmu)
  * Perform the group schedulability test as a whole
  * Return 0 if success
  */
-static int power_pmu_commit_txn(struct pmu *pmu)
+static int power_pmu_commit_txn(struct pmu *pmu, int flags)
 {
struct cpu_hw_events *cpuhw;
long i, n;
 
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return -EINVAL;
+
if (!ppmu)
return -EAGAIN;
cpuhw = this_cpu_ptr(cpu_hw_events);
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index b71a7f8..b2c9e3b 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1607,8 +1607,11 @@ static inline void x86_pmu_read(struct perf_event *event)
  * Set the flag to make pmu::enable() not perform the
  * schedulability test, it will be performed at commit time
  */
-static void x86_pmu_start_txn(struct pmu *pmu)
+static void x86_pmu_start_txn(struct pmu *pmu, int flags)
 {
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return;
+
perf_pmu_disable(pmu);
__this_cpu_or(cpu_hw_events.group_flag, PERF_EVENT_TXN);
__this_cpu_write(cpu_hw_events.n_txn, 0);
@@ -1619,8 +1622,11 @@ static void x86_pmu_start_txn(struct pmu *pmu)
  * Clear the flag and pmu::enable() will perform the
  * schedulability test.
  */
-static void x86_pmu_cancel_txn(struct pmu *pmu)
+static void x86_pmu_cancel_txn(struct pmu *pmu, int flags)
 {
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return;
+
__this_cpu_and(cpu_hw_events.group_flag, ~PERF_EVENT_TXN);
/*
 * Truncate collected array by the number of events added in this
@@ -1638,12 +1644,15 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
  *
  * Does not cancel the transaction on failure; expects the caller to do this.
  */
-static int x86_pmu_commit_txn(struct pmu *pmu)
+static int x86_pmu_commit_txn(struct pmu *pmu, int flags)
 {
struct cpu_hw_events *cpuc = this_cpu_ptr(cpu_hw_events);
int assign[X86_PMC_IDX_MAX];
int n, ret;
 
+   if (flags  ~PERF_PMU_TXN_ADD)
+   return -EINVAL;
+
n = cpuc-n_events;
 
if (!x86_pmu_initialized())
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2b62198..c8fe60e 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -225,6 +225,8 @@ struct pmu {
 * should stop the counter when perf_event_overflow() returns
 * !0. -start() will be used to continue.
 */
+#definePERF_PMU_TXN_ADD1
+#definePERF_PMU_TXN_READ   2
void (*start)   (struct perf_event *event, int flags);
void (*stop)(struct perf_event *event, int flags);
 
@@ -240,20 +242,26 @@ struct pmu {
 *
 * Start the transaction, after this -add() doesn't need to
 * do schedulability tests.
+*
+* Optional.
 */
-   void (*start_txn)