[PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable

Mark Rutland
2016-01-11T10:46:10Z

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.