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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250116114751.GJ8362@noisy.programming.kicks-ass.net>
Date: Thu, 16 Jan 2025 12:47:51 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: kan.liang@...ux.intel.com
Cc: mingo@...hat.com, acme@...nel.org, namhyung@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
	ak@...ux.intel.com, eranian@...gle.com, dapeng1.mi@...ux.intel.com
Subject: Re: [PATCH V9 3/3] perf/x86/intel: Support PEBS counters snapshotting

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

> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
> Reviewed-by: Ian Rogers <irogers@...gle.com>

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@...radead.org>
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> ---

> 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?