lkml.org 
[lkml]   [2018]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 6/6] arm64: perf: Add support for chaining counters
From
Date
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 = {
> };
>
> PMU_FORMAT_ATTR(event, "config:0-15");
> +PMU_FORMAT_ATTR(chain, "config1:0");
>
> static struct attribute *armv8_pmuv3_format_attrs[] = {
> &format_attr_event.attr,
> + &format_attr_chain.attr,
> NULL,
> };
>
> @@ -457,6 +459,12 @@ static struct attribute_group armv8_pmuv3_format_attr_group = {
> .attrs = armv8_pmuv3_format_attrs,
> };
>
> +static bool armv8pmu_event_is_chained(struct perf_event *event)
> +{
> + return event->attr.config1 & 0x1;
> +}
> +
> +
> /*
> * Perf Events' indices
> */
> @@ -512,6 +520,36 @@ static inline int armv8pmu_select_counter(int idx)
> return idx;
> }
>
> +static inline u32 armv8pmu_read_evcntr(int idx)
> +{
> + return (armv8pmu_select_counter(idx) == idx) ?
> + read_sysreg(pmxevcntr_el0) : 0;
> +}
> +
> +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?

> +
> + return (hi << 32) | lo;
> +}
> +
> +static inline u64 armv8pmu_read_hw_counter(struct perf_event *event)
> +{
> + int idx = event->hw.idx;
> +
> + return armv8pmu_event_is_chained(event) ?
> + armv8pmu_read_chain_counter(idx) : armv8pmu_read_evcntr(idx);
> +}
> +
> static inline u64 armv8pmu_read_counter(struct perf_event *event)
> {
> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> @@ -524,12 +562,37 @@ static inline u64 armv8pmu_read_counter(struct perf_event *event)
> smp_processor_id(), idx);
> else if (idx == ARMV8_IDX_CYCLE_COUNTER)
> value = read_sysreg(pmccntr_el0);
> - else if (armv8pmu_select_counter(idx) == idx)
> - value = read_sysreg(pmxevcntr_el0);
> + else
> + value = armv8pmu_read_hw_counter(event);
>
> return value;
> }
>
> +static inline void armv8pmu_write_evcntr(int idx, u32 value)
> +{
> + if (armv8pmu_select_counter(idx) == idx)
> + write_sysreg(value, pmxevcntr_el0);
> +}
> +
> +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.

> +}
> +
> +static inline void armv8pmu_write_hw_counter(struct perf_event *event,
> + u64 value)
> +{
> + int idx = event->hw.idx;
> +
> + if (armv8pmu_event_is_chained(event))
> + armv8pmu_write_chain_counter(idx, value);
> + else
> + armv8pmu_write_evcntr(idx, value);
> +}
> +
> static inline void armv8pmu_write_counter(struct perf_event *event, u64 value)
> {
> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> @@ -541,8 +604,8 @@ static inline void armv8pmu_write_counter(struct perf_event *event, u64 value)
> smp_processor_id(), idx);
> else if (idx == ARMV8_IDX_CYCLE_COUNTER)
> write_sysreg(value, pmccntr_el0);
> - else if (armv8pmu_select_counter(idx) == idx)
> - write_sysreg(value, pmxevcntr_el0);
> + else
> + armv8pmu_write_hw_counter(event, value);
> }
>
> static inline void armv8pmu_write_evtype(int idx, u32 val)
> @@ -553,6 +616,28 @@ static inline void armv8pmu_write_evtype(int idx, u32 val)
> }
> }
>
> +static inline void armv8pmu_write_event_type(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + int idx = hwc->idx;
> +
> + /*
> + * For chained events, write the high counter event type
> + * followed by the low counter.
> + */
> + if (armv8pmu_event_is_chained(event)) {
> + u32 chain_evt = ARMV8_PMUV3_PERFCTR_CHAIN;
> +
> + /* Set the filters as that of the main event for chain */
> + chain_evt |= hwc->config_base & ~ARMV8_PMU_EVTYPE_EVENT;
> + armv8pmu_write_evtype(idx, chain_evt);
> + isb();
> + idx--;
> + }
> +
> + armv8pmu_write_evtype(idx, hwc->config_base);
> +}
> +
> static inline int armv8pmu_enable_counter(int idx)
> {
> u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> @@ -560,6 +645,23 @@ static inline int armv8pmu_enable_counter(int idx)
> return idx;
> }
>
> +static inline void armv8pmu_enable_event_counter(struct perf_event *event)
> +{
> + int idx = event->hw.idx;
> +
> + /*
> + * For chained events, we enable the high counter followed by
> + * the low counter.
> + */
> + armv8pmu_enable_counter(idx);
> +
> + if (armv8pmu_event_is_chained(event)) {
> + isb();
> + armv8pmu_enable_counter(idx - 1);
> + }
> +
> +}
> +
> 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.

> + int idx = hwc->idx;
> +
> + /*
> + * Disable the low counter followed by the high counter
> + * for chained events.
> + */
> + if (armv8pmu_event_is_chained(event)) {
> + armv8pmu_disable_counter(idx - 1);
> + isb();
> + }
> +
> + armv8pmu_disable_counter(idx);
> +}
> +
> +
> static inline int armv8pmu_enable_intens(int idx)
> {
> u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> @@ -574,6 +694,12 @@ static inline int armv8pmu_enable_intens(int idx)
> return idx;
> }
>
> +static inline int armv8pmu_enable_event_irq(struct perf_event *event)
> +{
> + /* For chained events, enable the interrupt for only the high counter */
> + return armv8pmu_enable_intens(event->hw.idx);
> +}
> +
> static inline int armv8pmu_disable_intens(int idx)
> {
> u32 counter = ARMV8_IDX_TO_COUNTER(idx);
> @@ -586,6 +712,11 @@ static inline int armv8pmu_disable_intens(int idx)
> return idx;
> }
>
> +static inline int armv8pmu_disable_event_irq(struct perf_event *event)
> +{
> + return armv8pmu_disable_intens(event->hw.idx);
> +}
> +
> static inline u32 armv8pmu_getreset_flags(void)
> {
> u32 value;
> @@ -603,10 +734,8 @@ static inline u32 armv8pmu_getreset_flags(void)
> static void armv8pmu_enable_event(struct perf_event *event)
> {
> unsigned long flags;
> - struct hw_perf_event *hwc = &event->hw;
> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
> - int idx = hwc->idx;
>
> /*
> * Enable counter and interrupt, and set the counter to count
> @@ -617,22 +746,22 @@ static void armv8pmu_enable_event(struct perf_event *event)
> /*
> * Disable counter
> */
> - armv8pmu_disable_counter(idx);
> + armv8pmu_disable_event_counter(event);
>
> /*
> * Set event (if destined for PMNx counters).
> */
> - armv8pmu_write_evtype(idx, hwc->config_base);
> + armv8pmu_write_event_type(event);
>
> /*
> * Enable interrupt for this counter
> */
> - armv8pmu_enable_intens(idx);
> + armv8pmu_enable_event_irq(event);
>
> /*
> * Enable counter
> */
> - armv8pmu_enable_counter(idx);
> + armv8pmu_enable_event_counter(event);
>
> raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> }
> @@ -640,10 +769,8 @@ static void armv8pmu_enable_event(struct perf_event *event)
> static void armv8pmu_disable_event(struct perf_event *event)
> {
> unsigned long flags;
> - struct hw_perf_event *hwc = &event->hw;
> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
> struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events);
> - int idx = hwc->idx;
>
> /*
> * Disable counter and interrupt
> @@ -651,14 +778,14 @@ static void armv8pmu_disable_event(struct perf_event *event)
> raw_spin_lock_irqsave(&events->pmu_lock, flags);
>
> /*
> - * Disable counter
> + * Disable counter for this event
> */
> - armv8pmu_disable_counter(idx);
> + armv8pmu_disable_event_counter(event);
>
> /*
> - * Disable interrupt for this counter
> + * Disable interrupt for this event counter
> */
> - armv8pmu_disable_intens(idx);
> + armv8pmu_disable_event_irq(event);
>
> raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> }
> @@ -747,6 +874,39 @@ static void armv8pmu_stop(struct arm_pmu *cpu_pmu)
> raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> }
>
> +static int armv8pmu_get_single_idx(struct pmu_hw_events *cpuc,
> + struct arm_pmu *cpu_pmu)
> +{
> + int idx;
> +
> + for (idx = cpu_pmu->num_events - 1; idx >= ARMV8_IDX_COUNTER0; ++idx)
> + if (!test_and_set_bit(idx, cpuc->used_mask))
> + return idx;
> + return -EAGAIN;
> +}
> +
> +static int armv8pmu_get_chain_idx(struct pmu_hw_events *cpuc,
> + struct arm_pmu *cpu_pmu)
> +{
> + int idx;
> +
> + /*
> + * Chaining requires two consecutive event counters, where
> + * the lower idx must be even. We allocate chain events
> + * from the lower index (i.e, counter0) and the single events
> + * from the higher end to maximise the utilisation.
> + */
> + for (idx = ARMV8_IDX_COUNTER0 + 1; idx < cpu_pmu->num_events; idx += 2)
> + if (!test_and_set_bit(idx, cpuc->used_mask)) {
> + /* Check if the preceding even counter is available */
> + if (!test_and_set_bit(idx - 1, cpuc->used_mask))
> + return idx;
> + /* Release the Odd counter */
> + clear_bit(idx, cpuc->used_mask);
> + }
> + return -EAGAIN;
> +}
> +
> 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 :/

> + */
> if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
> if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask))
> return ARMV8_IDX_CYCLE_COUNTER;
> @@ -764,13 +927,21 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc,
> /*
> * Otherwise use events counters
> */
> - for (idx = ARMV8_IDX_COUNTER0; idx < cpu_pmu->num_events; ++idx) {
> - if (!test_and_set_bit(idx, cpuc->used_mask))
> - return idx;
> - }
> + idx = armv8pmu_event_is_chained(event) ?
> + armv8pmu_get_chain_idx(cpuc, cpu_pmu) :
> + armv8pmu_get_single_idx(cpuc, cpu_pmu);
>
> - /* The counters are all in use. */
> - return -EAGAIN;
> + return idx;
> +}
> +
> +static void armv8pmu_clear_event_idx(struct pmu_hw_events *cpuc,
> + struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> +
> + clear_bit(hwc->idx, cpuc->used_mask);
> + if (armv8pmu_event_is_chained(event))
> + clear_bit(hwc->idx - 1, cpuc->used_mask);
> }
>
> /*
> @@ -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.

Robin.

> + if (armv8pmu_event_is_chained(event))
> + return -EINVAL;
> event->hw.flags |= ARMPMU_EVT_LONG;
> + } else if (armv8pmu_event_is_chained(event)) {
> + event->hw.flags |= ARMPMU_EVT_LONG;
> + }
>
> /* Onl expose micro/arch events supported by this PMU */
> if ((hw_event_id > 0) && (hw_event_id < ARMV8_PMUV3_MAX_COMMON_EVENTS)
> @@ -954,6 +1131,7 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu)
> cpu_pmu->read_counter = armv8pmu_read_counter,
> cpu_pmu->write_counter = armv8pmu_write_counter,
> cpu_pmu->get_event_idx = armv8pmu_get_event_idx,
> + cpu_pmu->clear_event_idx = armv8pmu_clear_event_idx,
> cpu_pmu->start = armv8pmu_start,
> cpu_pmu->stop = armv8pmu_stop,
> cpu_pmu->reset = armv8pmu_reset,
>

\
 
 \ /
  Last update: 2018-05-18 15:50    [W:0.139 / U:96.296 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site