Re: [PATCH V5 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Apr 24, 2019 at 11:58:22AM +0100, Will Deacon wrote:
> Hi Frank,
> 
> On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote:
> > Add ddr performance monitor support for iMX8QXP
> > 
> > There are 4 counters for ddr perfomance events.
> > counter 0 is dedicated for cycles.
> > you choose any up to 3 no cycles events.
> 
> I was about to pick this up, but I still have a few questions/comments
> on the code:
> 
> > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > +{
> > +	int i;
> > +	u8 reg;
> > +	int val;
> > +	int counter;
> > +	struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > +	struct perf_event *event;
> > +
> > +	/* Only cycles counter overflowed can issue irq. all counters will
> > +	 * be stopped when cycles counter overflow. but other counter don't stop
> > +	 * when overflow happen. Update all of the local counter values,
> > +	 * then reset the cycles counter, so the others can continue
> > +	 * counting. cycles counter is fastest counter in all events. at last
> > +	 * 4 times than other counters.
> > +	 */
> > +	for (i = 0; i < NUM_COUNTER; i++) {
> > +
> > +		if (!pmu->active_events[i])
> > +			continue;
> > +
> > +		event = pmu->active_events[i];
> > +		counter = event->hw.idx;
> > +		reg = counter * 4 + COUNTER_CNTL;
> > +		val = readl(pmu->base + reg);
> 
> Does this read have a side-effect, or can it be removed given that you don't
> use val for anything else?
> 
> > +		ddr_perf_event_update(event);
> > +
> > +		if (counter == EVENT_CYCLES_COUNTER) {
> > +			ddr_perf_event_enable(pmu,
> > +					      EVENT_CYCLES_ID,
> > +					      EVENT_CYCLES_COUNTER,
> > +					      true);
> > +			ddr_perf_event_update(event);
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> What stops the IRQ handler running concurrently with perf callbacks? I can't
> see any locking here, or is the IRQ supposed to be affine to the same CPU
> that's handling the perf context?

To be correct, the IRQ affinity should be set to match the logical
affinity of the PMU, but it looks like that's not done at setup or
migrate time.

Frank, please take a look at what drivers/perf/arm-ccn.c does with teh
IRQ, including the use of IRQF_NOBALANCING | IRQF_NO_THREAD flags when
requesting the IRQ to begin with.

Thanks,
Mark.