Re: [PATCH V9 3/3] perf/x86/intel: Support PEBS counters snapshotting

Peter Zijlstra
2025-01-16T12:02:10Z

On Wed, Jan 15, 2025 at 10:43:18AM -0800, kan.liang@linux.intel.com wrote:

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 81b6ec8e824e..10ce80230ad3 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1294,6 +1294,19 @@ static inline void pebs_update_threshold(struct cpu_hw_events *cpuc)
> ds->pebs_interrupt_threshold = threshold;
> }
>
> +#define PEBS_DATACFG_CNTRS(x) \
> + ((x >> PEBS_DATACFG_CNTR_SHIFT) & PEBS_DATACFG_CNTR_MASK)
> +
> +#define PEBS_DATACFG_CNTR_BIT(x) \
> + (((1ULL << x) & PEBS_DATACFG_CNTR_MASK) << PEBS_DATACFG_CNTR_SHIFT)
> +
> +#define PEBS_DATACFG_FIX(x) \
> + ((x >> PEBS_DATACFG_FIX_SHIFT) & PEBS_DATACFG_FIX_MASK)
> +
> +#define PEBS_DATACFG_FIX_BIT(x) \
> + (((1ULL << (x - INTEL_PMC_IDX_FIXED)) & PEBS_DATACFG_FIX_MASK) \
> + << PEBS_DATACFG_FIX_SHIFT)

That INTEL_PMX_IDX_FIXED does not belong here, needs to be at the
callsite.

> @@ -1914,6 +1975,34 @@ static void adaptive_pebs_save_regs(struct pt_regs *regs,
> #endif
> }
>
> +static void intel_perf_event_pmc_to_count(struct perf_event *event, u64 pmc)
> +{
> + int shift = 64 - x86_pmu.cntval_bits;
> + struct hw_perf_event *hwc;
> + u64 delta, prev_pmc;
> +
> + /*
> + * The PEBS record doesn't shrink on pmu::del().
> + * See pebs_update_state().
> + * Ignore the non-exist event.
> + */
> + if (!event)
> + return;
> +
> + hwc = &event->hw;
> + prev_pmc = local64_read(&hwc->prev_count);
> +
> + /* Only update the count when the PMU is disabled */
> + WARN_ON(this_cpu_read(cpu_hw_events.enabled));
> + local64_set(&hwc->prev_count, pmc);
> +
> + delta = (pmc << shift) - (prev_pmc << shift);
> + delta >>= shift;
> +
> + local64_add(delta, &event->count);
> + local64_sub(delta, &hwc->period_left);
> +}

Name and function don't really match at this point. When I suggested
this function it returned a count value, now it's closer to
intel_perf_event_update_pmc() or somesuch.

> @@ -2049,6 +2138,61 @@ static void setup_pebs_adaptive_sample_data(struct perf_event *event,
> }
> }
>
> + if (format_group & (PEBS_DATACFG_CNTR | PEBS_DATACFG_METRICS)) {
> + struct pebs_cntr_header *cntr = next_record;
> + int bit;
> +
> + next_record += sizeof(struct pebs_cntr_header);
> +
> + /*
> + * The PEBS_DATA_CFG is a global register, which is the
> + * superset configuration for all PEBS events.
> + * For the PEBS record of non-sample-read group, ignore
> + * the counter snapshot fields.
> + */
> + if (!is_pebs_counter_event_group(event)) {
> + unsigned int nr;
> +
> + nr = bitmap_weight((unsigned long *)&cntr->cntr, INTEL_PMC_MAX_GENERIC) +
> + bitmap_weight((unsigned long *)&cntr->fixed, INTEL_PMC_MAX_FIXED);

hweight32 ?

> + if (cntr->metrics == INTEL_CNTR_METRICS)
> + nr += 2;
> + next_record += nr * sizeof(u64);
> + goto end_cntr;

Yuck, can't you break out stuff into a helper function to get rid of
that goto?

> + }
> +
> + for_each_set_bit(bit, (unsigned long *)&cntr->cntr, INTEL_PMC_MAX_GENERIC) {
> + intel_perf_event_pmc_to_count(cpuc->events[bit], *(u64 *)next_record);
> + next_record += sizeof(u64);
> + }
> +
> + for_each_set_bit(bit, (unsigned long *)&cntr->fixed, INTEL_PMC_MAX_FIXED) {
> + /* The slots event will be handled with perf_metric later */
> + if ((cntr->metrics == INTEL_CNTR_METRICS) &&
> + (bit + INTEL_PMC_IDX_FIXED == INTEL_PMC_IDX_FIXED_SLOTS)) {
> + next_record += sizeof(u64);
> + continue;
> + }
> + intel_perf_event_pmc_to_count(cpuc->events[bit + INTEL_PMC_IDX_FIXED],
> + *(u64 *)next_record);
> + next_record += sizeof(u64);
> + }
> +
> + /* HW will reload the value right after the overflow. */
> + if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
> + local64_set(&event->hw.prev_count, (u64)-event->hw.sample_period);
> +
> + if (cntr->metrics == INTEL_CNTR_METRICS) {
> + static_call(intel_pmu_update_topdown_event)
> + (cpuc->events[INTEL_PMC_IDX_FIXED_SLOTS],
> + (u64 *)next_record);
> + next_record += 2 * sizeof(u64);
> + }
> + data->sample_flags |= PERF_SAMPLE_READ;
> + }
> +
> +end_cntr:
> +
> WARN_ONCE(next_record != __pebs + basic->format_size,
> "PEBS record size %u, expected %llu, config %llx\n",
> basic->format_size,