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

[Peter Zijlstra]

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

> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Did they really read all the various versions without spotting any of
the problems, or are you just rubber stamping things at this point :/

Best to drop review tags after serious changes to patches.

> Suggested-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> ---

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 8f218ac0d445..79a4aad5a0a3 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -94,6 +94,8 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases, *x86_pmu.pebs_aliases);
>  
>  DEFINE_STATIC_CALL_NULL(x86_pmu_filter, *x86_pmu.filter);
>  
> +DEFINE_STATIC_CALL_NULL(x86_pmu_late_config, x86_pmu_late_config);

So all these static call are through struct x86_pmu (hence the naming),
but not this one ?!?

>  /*
>   * This one is magic, it will get called even when PMU init fails (because
>   * there is no PMU), in which case it should simply return NULL.
> @@ -1329,7 +1331,16 @@ static void x86_pmu_enable(struct pmu *pmu)
>  		}
>  
>  		/*
> -		 * step2: reprogram moved events into new counters
> +		 * step2:
> +		 * The late config (after counters are scheduled)
> +		 * is required for some cases, e.g., PEBS counters
> +		 * snapshotting. Because an accurate counter index
> +		 * is needed.
> +		 */
> +		static_call_cond(x86_pmu_late_config)();
> +
> +		/*
> +		 * step3: reprogram moved events into new counters
>  		 */
>  		for (i = 0; i < cpuc->n_events; i++) {
>  			event = cpuc->event_list[i];

Placement and naming are weird.

These 2 loops migrate all counters that got a new place, the first loop
taking out pre-existing counters that got a new place, the second loop
setting up these same and new counters.

Why do the pebs config in the middle, where the least number of counters
are set-up?

AFAICT all it really needs is cpuc->assignp[], which is unchanged
throughout. You can call this at any time before clearing n_added,
sticking it in the middle just seems weird.

Then naming, config is typically what we do at event creation,
x86_pmu_hw_config() like. This is not at all related, so perhaps pick a
different name?

Bah, we're so very close between x86_pmu::{enable, disable, assign}() :/

Also, I think I found you another bug... Consider what happens to the
counter value when we reschedule a HES_STOPPED counter, then we skip
x86_pmu_start(RELOAD) on step2, which leave the counter value with
'random' crap from whatever was there last.

But meanwhile you do program PEBS to sample it. That will happily sample
this garbage.

Hmm?