Re: [PATCH 2/4] perf/x86/amd/uncore: Use hrtimer for handling overflows

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Apr 09, 2025 at 01:27:07PM +0530, Sandipan Das wrote:
> Uncore counters do not provide mechanisms like interrupts to report
> overflows and the accumulated user-visible count is incorrect if there
> is more than one overflow between two successive read requests for the
> same event because the value of prev_count goes out-of-date for
> calculating the correct delta.
> 
> To avoid this, start a hrtimer to periodically initiate a pmu->read() of
> the active counters for keeping prev_count up-to-date. It should be
> noted that the hrtimer duration should be lesser than the shortest time
> it takes for a counter to overflow for this approach to be effective.
> 
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> ---
>  arch/x86/events/amd/uncore.c | 72 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 010024f09f2c..ee6528f2189f 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -21,6 +21,7 @@
>  #define NUM_COUNTERS_NB		4
>  #define NUM_COUNTERS_L2		4
>  #define NUM_COUNTERS_L3		6
> +#define NUM_COUNTERS_MAX	64
>  
>  #define RDPMC_BASE_NB		6
>  #define RDPMC_BASE_LLC		10
> @@ -38,6 +39,10 @@ struct amd_uncore_ctx {
>  	int refcnt;
>  	int cpu;
>  	struct perf_event **events;
> +	unsigned long active_mask[BITS_TO_LONGS(NUM_COUNTERS_MAX)];
> +	int nr_active;
> +	struct hrtimer hrtimer;
> +	u64 hrtimer_duration;
>  };
>  
>  struct amd_uncore_pmu {
> @@ -87,6 +92,51 @@ static struct amd_uncore_pmu *event_to_amd_uncore_pmu(struct perf_event *event)
>  	return container_of(event->pmu, struct amd_uncore_pmu, pmu);
>  }
>  
> +static enum hrtimer_restart amd_uncore_hrtimer(struct hrtimer *hrtimer)
> +{
> +	struct amd_uncore_ctx *ctx;
> +	struct perf_event *event;
> +	unsigned long flags;
> +	int bit;
> +
> +	ctx = container_of(hrtimer, struct amd_uncore_ctx, hrtimer);
> +
> +	if (!ctx->nr_active || ctx->cpu != smp_processor_id())
> +		return HRTIMER_NORESTART;
> +
> +	/*
> +	 * Disable local interrupts to prevent pmu->start() or pmu->stop()
> +	 * from interrupting the update process
> +	 */
> +	local_irq_save(flags);
> +
> +	for_each_set_bit(bit, ctx->active_mask, NUM_COUNTERS_MAX) {
> +		event = ctx->events[bit];
> +		event->pmu->read(event);
> +	}
> +
> +	local_irq_restore(flags);
> +
> +	hrtimer_forward_now(hrtimer, ns_to_ktime(ctx->hrtimer_duration));
> +	return HRTIMER_RESTART;
> +}
> +
> +static void amd_uncore_start_hrtimer(struct amd_uncore_ctx *ctx)
> +{
> +	hrtimer_start(&ctx->hrtimer, ns_to_ktime(ctx->hrtimer_duration),
> +		      HRTIMER_MODE_REL_PINNED);
> +}

So I know you copied this from the Intel uncore driver; but would not
both be improved by using HRTIMER_MODE_HARD?

It makes no sense to me to bounce the thing to SoftIRQ only to then
disable IRQs in the handler again. Not to mention that the whole SoftIRQ
things delays things further, giving more room/time to reach overflow
wrap.