[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5d53fe1d-dbd9-2aad-73bd-0c4284540240@arm.com>
Date: Fri, 18 May 2018 16:57:17 +0100
From: Suzuki K Poulose <Suzuki.Poulose@....com>
To: Robin Murphy <robin.murphy@....com>,
linux-arm-kernel@...ts.infradead.org
Cc: linux-kernel@...r.kernel.org, mark.rutland@....com,
will.deacon@....com
Subject: Re: [PATCH 6/6] arm64: perf: Add support for chaining counters
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@....com>
>> Cc: Will Deacon <will.deacon@....com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....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
Powered by blists - more mailing lists