Re: [PATCH] perf_event: e500 support

[Scott Wood <scottwood@freescale.com>]

Paul Mackerras wrote:
>> Some limitations:
>> - No threshold support -- need to figure out how to represent it in
>>   the event struct from userspace.
> 
> What does "threshold support" mean in this context?  Does it mean
> something different from getting an interrupt after N events have been
> counted?  Or does it mean counting instances where something takes
> longer than a specific number of cycles?

The latter.

>> - When trying to schedule multiple event groups at once, and using
>>   restricted events, situations could arise where scheduling fails even
>>   though it would be possible.  Consider three groups, each with two events.
>>   One group has restricted events, the others don't.  The two non-restricted
>>   groups are scheduled, then one is removed, which happens to occupy the two
>>   counters that can't do restricted events.  The remaining non-restricted
>>   group will not be moved to the non-restricted-capable counters to make
>>   room if the restricted group tries to be scheduled.  Since thresholds are
>>   not yet supported (though you can use the events with a threshold of
>>   zero), and threshold events are the only restricted events, this seems
>>   like a low priority issue.
> 
> Which way around are the restrictions?  That some events can only be
> counted on certain counters, or that some counters can only count a
> subset of the available events?

You could look at it either way -- threshold-capable events can only go on 
counters 0-1, or counters 2-3 can only count non-threshold-capable events.

> Did you look at the constraint satisfaction code in the existing
> perf_event.c and p*-pmu.c?  That lets you express both sorts of
> restrictions and automatically find the best solution (including
> moving events from one counter to another like you describe).

I did look at it -- but I had a hard time understanding it, and went with the 
simpler approach for now since the constraints are minimal on these chips.  I'm 
open to converting with a little help, if it doesn't add too much complexity or 
if future chips need it.

>> +#ifdef CONFIG_FSL_EMB_PERFMON
>> +#define MAX_HWEVENTS 4
>> +
>> +/* event flags */
>> +#define FSL_EMB_EVENT_VALID 1
>> +#define FSL_EMB_EVENT_RESTRICTED 2
>> +
>> +struct power_pmu {
> 
> I wonder if we should have just the stuff exported to the core in
> asm/perf_event.h and move MAX_HWEVENTS, struct power_pmu etc. to
> separate headers for fsl_embedded, classic, etc.?

Maybe.  We may also want to consider making it runtime selectable, as later 
revisions of e300 have an e500-style performance counter.  There's also a 
separate platform-level performance counter on some of our chips; having arch 
infrastructure that can delegate to multiple counter types would help there as well.

>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -98,7 +98,10 @@ obj64-$(CONFIG_AUDIT)		+= compat_audit.o
>>  
>>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
>>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
>> -obj-$(CONFIG_PPC_PERF_CTRS)	+= perf_event.o perf_callchain.o
>> +obj-$(CONFIG_PPC_PERF_CTRS)	+= perf_event.o
>> +obj-$(CONFIG_FSL_EMB_PERF_EVENT) += perf_event_fsl_emb.o
>> +obj-$(CONFIG_FSL_EMB_PERF_EVENT_E500) += e500-pmu.o
>> +obj-$(CONFIG_PERF_EVENTS)	+= perf_callchain.o
> 
> This is because we want perf_callchain.o even if we don't have
> hardware PMU support, is it?  If so this is a separate fix that
> deserves its own patch.

OK.

-Scott