| Message ID | 1450374559-23315-6-git-send-email-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 17, 2015 at 05:49:12PM +0000, Suzuki K. Poulose wrote: > This patch adds the transaction hooks for CCI PMU, which can be > later exploited to amortise the cost of writing the counters for > CCI-500 PMU. > > We keep track of only the 'ADD' transactions. While we are in a > transaction, we keep track of the indices allocated for the events > and delay the following operations until the transaction is committed. > 1) Programming the event on the counter > 2) Enabling the counter > 3) Setting the period for the event. So that's not really what the txn interface is for, its meant to amortize event scheduling. The above doesn't look like it has a failure case, in which case you can achieve the same simpler, using pmu::pmu_{dis,en}able().
On 17/12/15 18:42, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 05:49:12PM +0000, Suzuki K. Poulose wrote: >> We keep track of only the 'ADD' transactions. While we are in a >> transaction, we keep track of the indices allocated for the events >> and delay the following operations until the transaction is committed. >> 1) Programming the event on the counter >> 2) Enabling the counter >> 3) Setting the period for the event. > > So that's not really what the txn interface is for, its meant to > amortize event scheduling. OK > > The above doesn't look like it has a failure case, in which case you can > achieve the same simpler, using pmu::pmu_{dis,en}able(). > I thought about that, but was not sure if pmu->stop() is guaranteed to be called on all the events scheduled on the PMU when we pmu::pmu_disable(). Is it ? Thanks for the quick response. Suzuki
On Fri, Dec 18, 2015 at 10:28:23AM +0000, Suzuki K. Poulose wrote: > On 17/12/15 18:42, Peter Zijlstra wrote: > >The above doesn't look like it has a failure case, in which case you can > >achieve the same simpler, using pmu::pmu_{dis,en}able(). > > > > I thought about that, but was not sure if pmu->stop() is guaranteed to be > called on all the events scheduled on the PMU when we pmu::pmu_disable(). > Is it ? Not by core code, but you get to implement your pmu::pmu_disable() call, and if that's what you need, you can make it do that. Examples: On some x86 hardware we indeed have to poke at each counter control register and clear the ENable bit, which is the same what pmu::stop(.flags=0) would do. But other x86 hardware has a global disable switch, which is much cheaper than poking at the individual counter control registers one by one. In this case we only update the counter control register if it needs updates (typically in the pmu_enable path). Yet other x86 hardware can auto disable this global state on interrupt, which saves us yet another machine register poke.
On 18/12/15 10:42, Peter Zijlstra wrote: > On Fri, Dec 18, 2015 at 10:28:23AM +0000, Suzuki K. Poulose wrote: >> On 17/12/15 18:42, Peter Zijlstra wrote: >> I thought about that, but was not sure if pmu->stop() is guaranteed to be >> called on all the events scheduled on the PMU when we pmu::pmu_disable(). >> Is it ? > > Not by core code, but you get to implement your pmu::pmu_disable() call, > and if that's what you need, you can make it do that. OK. > > Examples: > > On some x86 hardware we indeed have to poke at each counter control > register and clear the ENable bit, which is the same what > pmu::stop(.flags=0) would do. We have a global Enable/Disable for CCI PMU and thats what we use currently. To be able to reprogram the counters with the event period (we program the counter with a specific count in pmu::start() and at overflow irq handler, not to be confused with the sampling period, which is not supported), we need to be sure that the counter value has been updated. May be we could check the event->hw->state to see if we need to reprogram it. Thanks Suzuki
On Fri, Dec 18, 2015 at 10:58:17AM +0000, Suzuki K. Poulose wrote: > We have a global Enable/Disable for CCI PMU and thats what we use > currently. To be able to reprogram the counters with the event period > (we program the counter with a specific count in pmu::start() and at > overflow irq handler, not to be confused with the sampling period, which > is not supported), we need to be sure that the counter value has been updated. > > May be we could check the event->hw->state to see if we need to reprogram it. Right, have a look at arch/x86/kernel/cpu/perf_event.c:x86_pmu_enable() If there's new events, it does two loops over the events. The first loop does stop(PERF_EF_UPDATE) any counter that got moved. The second loop does start(PERF_EF_RELOAD) on moved and new events. The PERF_HES_ARCH bit is used to preserve the stopped state of counters that were programmed but temporarily stopped.
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index f00cbce..ec3d4fd 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -115,6 +115,8 @@ struct cci_pmu_hw_events { struct perf_event **events; unsigned long *used_mask; raw_spinlock_t pmu_lock; + unsigned long txn_flags; + unsigned long *txn_mask; }; struct cci_pmu; @@ -965,12 +967,25 @@ static void cci_pmu_start(struct perf_event *event, int pmu_flags) raw_spin_lock_irqsave(&hw_events->pmu_lock, flags); - /* Configure the counter unless you are counting a fixed event */ - if (!pmu_fixed_hw_idx(cci_pmu, idx)) - pmu_set_event(cci_pmu, idx, hwc->config_base); + /* + * If we got here from pmu->add(PERF_EF_START) while we are in a + * transaction, we note down the index and write to the counters + * in a batch when we commit the transaction. see cci_pmu_commit_txn(). + * Also, mark this one as STOPPED until we commit the transaction + * to avoid reading bogus values in pmu->del() if the transaction + * fails later. + */ + if ((pmu_flags & PERF_EF_START) && (hw_events->txn_flags == PERF_PMU_TXN_ADD)) { + hwc->state = PERF_HES_STOPPED; + set_bit(idx, hw_events->txn_mask); + } else { + /* Configure the counter unless you are counting a fixed event */ + if (!pmu_fixed_hw_idx(cci_pmu, idx)) + pmu_set_event(cci_pmu, idx, hwc->config_base); - pmu_event_set_period(event); - pmu_enable_counter(cci_pmu, idx); + pmu_event_set_period(event); + pmu_enable_counter(cci_pmu, idx); + } raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags); } @@ -981,6 +996,10 @@ static void cci_pmu_stop(struct perf_event *event, int pmu_flags) struct hw_perf_event *hwc = &event->hw; int idx = hwc->idx; + /* + * If the counter was never started, e.g a failed transaction + * do nothing. + */ if (hwc->state & PERF_HES_STOPPED) return; @@ -1200,6 +1219,87 @@ static int cci_pmu_event_init(struct perf_event *event) return err; } +static void cci_pmu_start_txn(struct pmu *pmu, unsigned int txn_flags) +{ + struct cci_pmu *cci_pmu = to_cci_pmu(pmu); + struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events; + + WARN_ON_ONCE(hw_events->txn_flags); + + hw_events->txn_flags = txn_flags; + memset(hw_events->txn_mask, 0, + BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); +} + +/* + * Completing the transaction involves : + * + * 1) Updating the period for each event in the transaction. + * - Updating the event->hw.prev_count for each event. + * - Writing the period to all the counters allocated for + * the transaction. + * 2) Program the events to the counters + * 3) Changing the event->hw.state from PERF_HES_STOPPED, now that + * we are committing the event. + * 4) Enable the counter + */ +static int cci_pmu_complete_txn(struct cci_pmu *cci_pmu) +{ + int i, rc = 0; + unsigned long flags; + struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events; + + raw_spin_lock_irqsave(&hw_events->pmu_lock, flags); + + /* Set event period for all the counters in this txn */ + pmu_write_counters(cci_pmu, hw_events->txn_mask, CCI_CNTR_PERIOD); + + for_each_set_bit(i, hw_events->txn_mask, cci_pmu->num_cntrs) { + struct perf_event *event = hw_events->events[i]; + + if (!event) { + WARN_ON_ONCE(1); + rc = -EFAULT; + goto unlock; + } + + local64_set(&event->hw.prev_count, CCI_CNTR_PERIOD); + if (!pmu_fixed_hw_idx(cci_pmu, i)) + pmu_set_event(cci_pmu, i, event->hw.config_base); + event->hw.state = 0; + pmu_enable_counter(cci_pmu, i); + } + +unlock: + raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags); + + return rc; +} + +static int cci_pmu_commit_txn(struct pmu *pmu) +{ + int rc = 0; + struct cci_pmu *cci_pmu = to_cci_pmu(pmu); + struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events; + + WARN_ON_ONCE(!hw_events->txn_flags); + + if (hw_events->txn_flags == PERF_PMU_TXN_ADD) + rc = cci_pmu_complete_txn(cci_pmu); + + if (!rc) + hw_events->txn_flags = 0; + return rc; +} + +static void cci_pmu_cancel_txn(struct pmu *pmu) +{ + struct cci_pmu_hw_events *hw_events = &to_cci_pmu(pmu)->hw_events; + + WARN_ON_ONCE(!hw_events->txn_flags); + hw_events->txn_flags = 0; +} + static ssize_t pmu_cpumask_attr_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -1257,6 +1357,9 @@ static int cci_pmu_init(struct cci_pmu *cci_pmu, struct platform_device *pdev) .pmu_enable = cci_pmu_enable, .pmu_disable = cci_pmu_disable, .event_init = cci_pmu_event_init, + .start_txn = cci_pmu_start_txn, + .commit_txn = cci_pmu_commit_txn, + .cancel_txn = cci_pmu_cancel_txn, .add = cci_pmu_add, .del = cci_pmu_del, .start = cci_pmu_start, @@ -1463,6 +1566,12 @@ static struct cci_pmu *cci_pmu_alloc(struct platform_device *pdev) if (!cci_pmu->hw_events.used_mask) return ERR_PTR(-ENOMEM); + cci_pmu->hw_events.txn_mask = devm_kcalloc(&pdev->dev, + BITS_TO_LONGS(CCI_PMU_MAX_HW_CNTRS(model)), + sizeof(*cci_pmu->hw_events.txn_mask), + GFP_KERNEL); + if (!cci_pmu->hw_events.txn_mask) + return ERR_PTR(-ENOMEM); return cci_pmu; }
This patch adds the transaction hooks for CCI PMU, which can be later exploited to amortise the cost of writing the counters for CCI-500 PMU. We keep track of only the 'ADD' transactions. While we are in a transaction, we keep track of the indices allocated for the events and delay the following operations until the transaction is committed. 1) Programming the event on the counter 2) Enabling the counter 3) Setting the period for the event. Additionally to prevent pmu->del() from updating bogus values from an event added in the transaction (since we haven't set the period on the event before the transaction is committed), we mark the state of the event as PERF_HES_STOPPED in pmu->start(). This will be cleared once the transaction is committed. Cc: Mark Rutland <mark.rutland@arm.com> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: peterz@infradead.org Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com> --- drivers/bus/arm-cci.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 114 insertions(+), 5 deletions(-)