Re: [PATCH V2] perf/imx_ddr: Add stop event counters support for i.MX8MP

[Will Deacon <will@kernel.org>]

On Mon, Sep 21, 2020 at 07:36:02PM +0100, Will Deacon wrote:
> On Tue, Sep 08, 2020 at 06:47:34PM +0800, Joakim Zhang wrote:
> > +static void ddr_perf_event_update(struct perf_event *event)
> > +{
> > +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	u64 delta, prev_raw_count, new_raw_count;
> > +	int counter = hwc->idx;
> > +	int ret;
> > +
> > +	if (counter == EVENT_CYCLES_COUNTER) {
> > +		do {
> > +			prev_raw_count = local64_read(&hwc->prev_count);
> > +			new_raw_count = ddr_perf_read_counter(pmu, counter);
> > +		} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> > +				new_raw_count) != prev_raw_count);
> > +
> > +		delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF;
> > +
> > +		local64_add(delta, &event->count);
> 
> Why do we treat the cycle counter so differently here?

Ah, please can you remind me: does the cycle counter stop on overflow, or
does it continue to count?

> > +	} else {
> > +		/*
> > +		 * For legacy SoCs: event counters continue counting when overflow,
> > +		 *                  no need to clear the counter.
> > +		 * For new SoCs: event counters stop counting when overflow, need
> > +		 *               clear counter to let it count again.
> > +		 */
> > +		ret = ddr_perf_counter_overflow(pmu, counter);
> > +		if (ret)
> > +			dev_warn(pmu->dev, "Event Counter%d overflow happened, data incorrect!!\n", counter);
> 
> I don't understand this message: if the data is incorrect, why do we need
> to handle overflow at all/, rather than putting the event into an error
> state?

Also, now I remember how this all works (we use the cycle counter to stop
overflow of the event counter), then warning here is ok, but we should do
something like:

	dev_warn_ratelimited(pmu->dev, "events lost due to counter overflow (config 0x%llx)\n", event->attr.config);

instead.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel