Messages in this thread Patch in this message | | | Date | Mon, 11 Jan 2016 10:46:10 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH v5 05/11] arm-cci PMU: Delay counter writes to pmu_enable |
| |
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.
| |