| Message ID | 1451908490-2615-6-git-send-email-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: > Delay setting the event periods for enabled events to pmu::pmu_enable(). > We mark the event.hw->state PERF_HES_ARCH for the events that we know > have their counts recorded and have been started. Please add a comment to the code stating exactly what PERF_HES_ARCH means for the CCI PMU driver, so it's easy to find. > Since we reprogram the counters every time before count, we can set > the counters for all the event counters which are !STOPPED && ARCH. > > Grouping the writes to counters can ammortise the cost of the operation > on PMUs where it is expensive (e.g, CCI-500). > > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Punit Agrawal <punit.agrawal@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com> > --- > drivers/bus/arm-cci.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c > index 0189f3a..c768ee4 100644 > --- a/drivers/bus/arm-cci.c > +++ b/drivers/bus/arm-cci.c > @@ -916,6 +916,40 @@ static void hw_perf_event_destroy(struct perf_event *event) > } > } > > +/* > + * Program the CCI PMU counters which have PERF_HES_ARCH set > + * with the event period and mark them ready before we enable > + * PMU. > + */ > +void cci_pmu_update_counters(struct cci_pmu *cci_pmu) > +{ > + int i; > + unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)]; I think this can be: DECLARE_BITMAP(mask, cci_pmu->num_cntrs); > + > + memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); Likewise: bitmap_zero(mask, cci_pmu->num_cntrs); > + > + for_each_set_bit(i, cci_pmu->hw_events.used_mask, cci_pmu->num_cntrs) { > + struct hw_perf_event *hwe; > + > + if (!cci_pmu->hw_events.events[i]) { > + WARN_ON(1); > + continue; > + } > + if (WARN_ON(!cci_pmu->hw_events.events[i])) continue; > + hwe = &cci_pmu->hw_events.events[i]->hw; > + /* Leave the events which are not counting */ > + if (hwe->state & PERF_HES_STOPPED) > + continue; > + if (hwe->state & PERF_HES_ARCH) { > + set_bit(i, mask); > + hwe->state &= ~PERF_HES_ARCH; > + local64_set(&hwe->prev_count, CCI_CNTR_PERIOD); > + } > + } > + > + pmu_write_counters(cci_pmu, mask, CCI_CNTR_PERIOD); > +} > + > static void cci_pmu_enable(struct pmu *pmu) > { > struct cci_pmu *cci_pmu = to_cci_pmu(pmu); > @@ -927,6 +961,7 @@ static void cci_pmu_enable(struct pmu *pmu) > return; > > raw_spin_lock_irqsave(&hw_events->pmu_lock, flags); > + cci_pmu_update_counters(cci_pmu); > __cci_pmu_enable(); > raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags); > > @@ -980,8 +1015,11 @@ static void cci_pmu_start(struct perf_event *event, int pmu_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); > - > - pmu_event_set_period(event); > + /* > + * Mark this counter, so that we can program the > + * counter with the event_period. see cci_pmu_enable() > + */ > + hwc->state = PERF_HES_ARCH; Why couldn't we have kept pmu_event_set_period here, and have that set prev_count and PERF_HES_ARCH? Then we'd be able to do the same betching for overflow too. What am I missing? Mark. > pmu_enable_counter(cci_pmu, idx); > > raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags); > -- > 1.7.9.5 >
On 04/01/16 19:24, Mark Rutland wrote: > On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: >> Delay setting the event periods for enabled events to pmu::pmu_enable(). >> We mark the event.hw->state PERF_HES_ARCH for the events that we know >> have their counts recorded and have been started. > > Please add a comment to the code stating exactly what PERF_HES_ARCH > means for the CCI PMU driver, so it's easy to find. > Sure. >> +void cci_pmu_update_counters(struct cci_pmu *cci_pmu) >> +{ >> + int i; >> + unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)]; > > I think this can be: > > DECLARE_BITMAP(mask, cci_pmu->num_cntrs); > >> + >> + memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); > > Likewise: > > bitmap_zero(mask, cci_pmu->num_cntrs); OK >> + if (!cci_pmu->hw_events.events[i]) { >> + WARN_ON(1); >> + continue; >> + } >> + > > if (WARN_ON(!cci_pmu->hw_events.events[i])) > continue; OK >> @@ -980,8 +1015,11 @@ static void cci_pmu_start(struct perf_event *event, int pmu_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); >> - >> - pmu_event_set_period(event); >> + /* >> + * Mark this counter, so that we can program the >> + * counter with the event_period. see cci_pmu_enable() >> + */ >> + hwc->state = PERF_HES_ARCH; > > Why couldn't we have kept pmu_event_set_period here, and have that set > prev_count and PERF_HES_ARCH? > > Then we'd be able to do the same betching for overflow too. The pmu is not disabled while we are in overflow irq handler. Hence there may not be a pmu_enable() which would set the period for the counter which overflowed, if defer the write in that case. Is that assumption wrong ? Cheers Suzuki
On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote: > On 04/01/16 19:24, Mark Rutland wrote: > >On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: > >>Delay setting the event periods for enabled events to pmu::pmu_enable(). > >>We mark the event.hw->state PERF_HES_ARCH for the events that we know > >>have their counts recorded and have been started. > > > >Please add a comment to the code stating exactly what PERF_HES_ARCH > >means for the CCI PMU driver, so it's easy to find. > > > > Sure. > > >>+void cci_pmu_update_counters(struct cci_pmu *cci_pmu) > >>+{ > >>+ int i; > >>+ unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)]; > > > >I think this can be: > > > > DECLARE_BITMAP(mask, cci_pmu->num_cntrs); > > > >>+ > >>+ memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); > > > >Likewise: > > > > bitmap_zero(mask, cci_pmu->num_cntrs); > > OK > > >>+ if (!cci_pmu->hw_events.events[i]) { > >>+ WARN_ON(1); > >>+ continue; > >>+ } > >>+ > > > > if (WARN_ON(!cci_pmu->hw_events.events[i])) > > continue; > > OK > >>@@ -980,8 +1015,11 @@ static void cci_pmu_start(struct perf_event *event, int pmu_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); > >>- > >>- pmu_event_set_period(event); > >>+ /* > >>+ * Mark this counter, so that we can program the > >>+ * counter with the event_period. see cci_pmu_enable() > >>+ */ > >>+ hwc->state = PERF_HES_ARCH; > > > >Why couldn't we have kept pmu_event_set_period here, and have that set > >prev_count and PERF_HES_ARCH? > > > >Then we'd be able to do the same betching for overflow too. > > The pmu is not disabled while we are in overflow irq handler. Hence there may > not be a pmu_enable() which would set the period for the counter which > overflowed, if defer the write in that case. Is that assumption wrong ? As the driver stands today, yes. However, wouldn't it make more sense to disable the PMU for the overflow handler, such that we can reuse the batching logic? Thanks, Mark.
On 11/01/16 10:46, Mark Rutland wrote: > On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote: >> On 04/01/16 19:24, Mark Rutland wrote: >>> On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: >> The pmu is not disabled while we are in overflow irq handler. Hence there may >> not be a pmu_enable() which would set the period for the counter which >> overflowed, if defer the write in that case. Is that assumption wrong ? > > As the driver stands today, yes. > > However, wouldn't it make more sense to disable the PMU for the overflow > handler, such that we can reuse the batching logic? None of the PMU drivers do that AFAIK. Hence, didn't want to change it for CCI. We could use the batching logic, if decide to do so. I can go ahead with that if there are no other side effects with that. Suzuki
On Mon, Jan 11, 2016 at 11:08:27AM +0000, Suzuki K. Poulose wrote: > On 11/01/16 10:46, Mark Rutland wrote: > >On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote: > >>On 04/01/16 19:24, Mark Rutland wrote: > >>>On Mon, Jan 04, 2016 at 11:54:44AM +0000, Suzuki K. Poulose wrote: > >>The pmu is not disabled while we are in overflow irq handler. Hence there may > >>not be a pmu_enable() which would set the period for the counter which > >>overflowed, if defer the write in that case. Is that assumption wrong ? > > > >As the driver stands today, yes. > > > >However, wouldn't it make more sense to disable the PMU for the overflow > >handler, such that we can reuse the batching logic? > > None of the PMU drivers do that AFAIK. I see. The Intel PMU driver disables the PMU for the interrupt handler; see intel_pmu_handle_irq in arch/x86/kernel/cpu/perf_event_intel.c. It looks like that's a special-case for sampling. I guess we may have the only case where it makes sense to batch counter writes as opposed to batching configuration writes. > Hence, didn't want to change it for CCI. We could use the batching > logic, if decide to do so. I can go ahead with that if there are no > other side effects with that. We'll lose events regardless as our RMW sequence will race against the counters. Batching will make that window slightly larger, but other than that I don't see a problem. Thanks, Mark.
On 11/01/16 11:24, Mark Rutland wrote: > On Mon, Jan 11, 2016 at 11:08:27AM +0000, Suzuki K. Poulose wrote: >> On 11/01/16 10:46, Mark Rutland wrote: >>> On Tue, Jan 05, 2016 at 09:59:13AM +0000, Suzuki K. Poulose wrote: >>>> On 04/01/16 19:24, Mark Rutland wrote: >> Hence, didn't want to change it for CCI. We could use the batching >> logic, if decide to do so. I can go ahead with that if there are no >> other side effects with that. > > We'll lose events regardless as our RMW sequence will race against the > counters. Batching will make that window slightly larger, but other than > that I don't see a problem. OK, will do that. Cheers Suzuki
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 0189f3a..c768ee4 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -916,6 +916,40 @@ static void hw_perf_event_destroy(struct perf_event *event) } } +/* + * Program the CCI PMU counters which have PERF_HES_ARCH set + * with the event period and mark them ready before we enable + * PMU. + */ +void cci_pmu_update_counters(struct cci_pmu *cci_pmu) +{ + int i; + unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)]; + + memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long)); + + for_each_set_bit(i, cci_pmu->hw_events.used_mask, cci_pmu->num_cntrs) { + struct hw_perf_event *hwe; + + if (!cci_pmu->hw_events.events[i]) { + WARN_ON(1); + continue; + } + + hwe = &cci_pmu->hw_events.events[i]->hw; + /* Leave the events which are not counting */ + if (hwe->state & PERF_HES_STOPPED) + continue; + if (hwe->state & PERF_HES_ARCH) { + set_bit(i, mask); + hwe->state &= ~PERF_HES_ARCH; + local64_set(&hwe->prev_count, CCI_CNTR_PERIOD); + } + } + + pmu_write_counters(cci_pmu, mask, CCI_CNTR_PERIOD); +} + static void cci_pmu_enable(struct pmu *pmu) { struct cci_pmu *cci_pmu = to_cci_pmu(pmu); @@ -927,6 +961,7 @@ static void cci_pmu_enable(struct pmu *pmu) return; raw_spin_lock_irqsave(&hw_events->pmu_lock, flags); + cci_pmu_update_counters(cci_pmu); __cci_pmu_enable(); raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags); @@ -980,8 +1015,11 @@ static void cci_pmu_start(struct perf_event *event, int pmu_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); - - pmu_event_set_period(event); + /* + * Mark this counter, so that we can program the + * counter with the event_period. see cci_pmu_enable() + */ + hwc->state = PERF_HES_ARCH; pmu_enable_counter(cci_pmu, idx); raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
Delay setting the event periods for enabled events to pmu::pmu_enable(). We mark the event.hw->state PERF_HES_ARCH for the events that we know have their counts recorded and have been started. Since we reprogram the counters every time before count, we can set the counters for all the event counters which are !STOPPED && ARCH. Grouping the writes to counters can ammortise the cost of the operation on PMUs where it is expensive (e.g, CCI-500). Cc: Mark Rutland <mark.rutland@arm.com> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com> --- drivers/bus/arm-cci.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)