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

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

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>
>> ---
>>   arch/arm64/kernel/perf_event.c | 226 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 202 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index ea8e060..5f81cd0 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -446,9 +446,11 @@ static struct attribute_group armv8_pmuv3_events_attr_group = {

..

>> +static inline u64 armv8pmu_read_chain_counter(int idx)
>> +{
>> +    u64 prev_hi, hi, lo;
>> +
>> +    do {
>> +        prev_hi = armv8pmu_read_evcntr(idx);
>> +        isb();
>> +        lo = armv8pmu_read_evcntr(idx - 1);
>> +        isb();
>> +        hi = armv8pmu_read_evcntr(idx);
>> +        isb();
>> +    } while (prev_hi != hi);
> 
> Is it worth trying to elide that last isb() in the highly likely case that we don't need it?

You're right. Also, I will rework the code to reuse the "hi".

>> +static inline void armv8pmu_write_chain_counter(int idx, u64 value)
>> +{
>> +    armv8pmu_write_evcntr(idx, value >> 32);
>> +    isb();
>> +    armv8pmu_write_evcntr(idx - 1, value);
>> +    isb();
> 
> Either that isb() is unnecessary, or we are (and have been) missing one after a non-chained write.

Thats right, it is not necessary, will remove it.


>>   static inline int armv8pmu_disable_counter(int idx)
>>   {
>>       u32 counter = ARMV8_IDX_TO_COUNTER(idx);
>> @@ -567,6 +669,24 @@ static inline int armv8pmu_disable_counter(int idx)
>>       return idx;
>>   }
>> +static inline void armv8pmu_disable_event_counter(struct perf_event *event)
>> +{
>> +    struct hw_perf_event *hwc = &event->hw;
> 
> Nit: might as well drop this and be consistent with the enable case.

Sure.

>>   static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
>>                     struct perf_event *event)
>>   {
>> @@ -755,7 +915,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
>>       struct hw_perf_event *hwc = &event->hw;
>>       unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT;
>> -    /* Always prefer to place a cycle counter into the cycle counter. */
>> +    /*
>> +     * Always prefer to place a cycle counter into the cycle counter
>> +     * irrespective of whether we are counting 32bit/64bit
> 
> I don't think that comment change adds much :/
> 

Thats a left over from rebasing. Thanks for spotting.

>>   /*
>> @@ -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.

Suzuki