Re: [RFC 2/2] powerpc/8xx: Perf events on PPC 8xx

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Dec 13, 2016 at 07:19:43PM +0100, Christophe Leroy wrote:
> +int mpc8xx_pmu_event_init(struct perf_event *event)
> +{
> +	int type = event_type(event);
> +
> +	switch (type) {
> +	case PERF_8xx_ID_CPU_CYCLES:
> +	case PERF_8xx_ID_ITLB_LOAD_MISS:
> +	case PERF_8xx_ID_DTLB_LOAD_MISS:
> +		break;
> +	case PERF_8xx_ID_HW_INSTRUCTIONS:
> +		mtspr(SPRN_CMPA, 0);

Should that not live in init_mpc8xx_pmu() ?

Afaict its a one time setup thing.

> +		break;
> +	default:
> +		return type;
> +	}
> +	return 0;
> +}
> +
> +int mpc8xx_pmu_add(struct perf_event *event, int flags)
> +{
> +	int type = event_type(event);
> +	s64 val;
> +
> +	switch (type) {
> +	case PERF_8xx_ID_CPU_CYCLES:
> +		val = get_tb();
> +		break;
> +	case PERF_8xx_ID_HW_INSTRUCTIONS:
> +		val = (instruction_counter << 16) | 0xffff;
> +		mtspr(SPRN_COUNTA, 0xffff0001);
> +		mtspr(SPRN_ICTRL, 0xc0080007);
> +		break;

So this sets up the counter and starts counting, right?

What happens if someone adds two events of this type?

Also, typical implementations would do:

	if (flags & PERF_EF_START)
		mpc8xx_pmu_start(event, flags);


> +	case PERF_8xx_ID_ITLB_LOAD_MISS:
> +		val = itlb_miss_counter;
> +		break;
> +	case PERF_8xx_ID_DTLB_LOAD_MISS:
> +		val = dtlb_miss_counter;
> +		break;
> +	default:
> +		break;
> +	}
> +	local64_set(&event->hw.prev_count, val);

Right, so aside from that INSTRUCTION event, the rest is treated like
freerunning counters which is fine.

> +	return 0;
> +}
> +
> +void mpc8xx_pmu_read(struct perf_event *event)
> +{
> +	int type = event_type(event);
> +	s64 prev, val, delta;
> +
> +	prev = local64_read(&event->hw.prev_count);
> +	switch (type) {
> +	case PERF_8xx_ID_CPU_CYCLES:
> +		val = get_tb();
> +		delta = 16 * (val - prev);
> +		break;
> +	case PERF_8xx_ID_HW_INSTRUCTIONS:
> +		mtspr(SPRN_ICTRL, 7);
> +		val = (instruction_counter << 16) | (0xffff - (mfspr(SPRN_COUNTA) >> 16));
> +		mtspr(SPRN_ICTRL, 0xc0080007);
> +		delta = val - prev;
> +		break;
> +	case PERF_8xx_ID_ITLB_LOAD_MISS:
> +		val = itlb_miss_counter;
> +		delta = val - prev;
> +		break;
> +	case PERF_8xx_ID_DTLB_LOAD_MISS:
> +		val = dtlb_miss_counter;
> +		delta = val - prev;
> +		break;
> +	default:
> +		break;
> +	}
> +	local64_set(&event->hw.prev_count, val);
> +	local64_add(delta, &event->count);
> +}

So there is one case, where we group this event with a software hrtimer
event and the hrtimer would call ::read() from interrupt context while
you're already in ::read().

This seems to suggest you still need a cmpxchg() loop to update, no?

> +void mpc8xx_pmu_del(struct perf_event *event, int flags)
> +{
> +	int type = event_type(event);
> +	s64 prev, val;
> +
> +	prev = local64_read(&event->hw.prev_count);
> +	switch (type) {
> +	case PERF_8xx_ID_HW_INSTRUCTIONS:
> +		mtspr(SPRN_ICTRL, 7);
> +		val = (instruction_counter << 16) | (0xffff - (mfspr(SPRN_COUNTA) >> 16));
> +		local64_add(val - prev, &event->count);
> +		break;
> +	case PERF_8xx_ID_CPU_CYCLES:
> +	case PERF_8xx_ID_ITLB_LOAD_MISS:
> +	case PERF_8xx_ID_DTLB_LOAD_MISS:
> +		mpc8xx_pmu_read(event);
> +		break;
> +	default:
> +		break;
> +	}

Right, so you make all del()'s imply PERF_EF_UPDATE, which is fine.

> +}
> +
> +void mpc8xx_pmu_start(struct perf_event *event, int flags)
> +{
> +}
> +
> +void mpc8xx_pmu_stop(struct perf_event *event, int flags)
> +{
> +}

So I think you can get away with doing this because the PMU doesn't do
sampling, which is what would normally start/stop already programmed
counters.

> +static struct pmu mpc8xx_pmu = {
> +	.event_init	= mpc8xx_pmu_event_init,
> +	.add		= mpc8xx_pmu_add,
> +	.del		= mpc8xx_pmu_del,
> +	.start		= mpc8xx_pmu_start,
> +	.stop		= mpc8xx_pmu_stop,
> +	.read		= mpc8xx_pmu_read,

	.capabilities	= PERF_PMU_CAP_NO_INTERRUPT |
			  PERF_PMU_CAP_NO_NMI,

> +};
> +
> +static int init_mpc8xx_pmu(void)
> +{
> +	return perf_pmu_register(&mpc8xx_pmu, "cpu", PERF_TYPE_RAW);
> +}
> +
> +early_initcall(init_mpc8xx_pmu);