Re: [PATCH 6/6] arm64: perf: Add support for chaining counters

[Robin Murphy <robin.murphy@arm.com>]

On 21/05/18 14:42, Suzuki K Poulose wrote:
> On 18/05/18 16:57, Suzuki K Poulose wrote:
>> Hi Robin,
>>
>> On 18/05/18 14:49, Robin Murphy wrote:
>>> On 18/05/18 11:22, Suzuki K Poulose wrote:
>>>> Add support for chained event counters. PMUv3 allows chaining
>>>> a pair of adjacent PMU counters (with the lower counter number
>>>> being always "even"). The low counter is programmed to count
>>>> the event of interest and the high counter(odd numbered) is
>>>> programmed with a special event code (0x1e - Chain). Thus
>>>> we need special allocation schemes to make the full use of
>>>> available counters. So, we allocate the counters from either
>>>> ends. i.e, chained counters are allocated from the lower
>>>> end in pairs of two and the normal counters are allocated
>>>> from the higher number. Also makes necessary changes to
>>>> handle the chained events as a single event with 2 counters.
>>>>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
>>
>>>>   /*
>>>> @@ -845,8 +1016,14 @@ static int __armv8_pmuv3_map_event(struct 
>>>> perf_event *event,
>>>>                          &armv8_pmuv3_perf_cache_map,
>>>>                          ARMV8_PMU_EVTYPE_EVENT);
>>>> -    if (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES)
>>>> +    if (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
>>>> +        /* Prevent chaining for cycle counter */
>>>
>>> Why? Sure, we want to avoid executing the chaining logic if we're 
>>> scheduling a cycles event in the dedicated counter (which is perhaps 
>>> what the comment above wanted to say), but if one ends up allocated 
>>> into a regular counter (e.g. if the user asks for multiple cycle 
>>> counts with different filters), then I don't see any reason to forbid 
>>> that being chained.
>>
>> Ah, I didn't think about that case. I was under the assumption that the
>> cycles are *only* placed on the cycle counter. I will take care of that.
>> Thanks for the review.
> 
> Robin, Mark, Will
> 
> One potential problem I see with allowing chaining of the cycle counter
> *and* the promotion of cycle event to 64bit by default is when the user
> may actually be able to count 1 less event (due to the promotion of
> cycle event to 64bit and thus forcing to use chain, if the cycle counter
> is unavailable).

Right, I didn't mean to imply we should inject the "chain" attr 
automatically for all cycles events, just that we shouldn't be rejecting 
it if the user does explicitly set it (but then just ignore it if using 
the dedicated counter).

> So one option is to drop automatic promotion of the cycle counter to
> 64bit and do it only when it is requested by the user and use either the
> Cycle counter (preferred) or fall back to chaining. That way, the user
> has the control over the number of events he can count using the given
> set of counters.

Naively, there doesn't seem to be any inherent harm in always using 
64-bit arithmetic for the dedicated counter, but it would mean that with 
multiple (non-chained) cycles events, some would be taking an interrupt 
every few seconds while one would effectively never overflow. I guess 
the question is whether that matters or not.

Robin.