linux-kernel - Re: [PATCH 2/4] perf/x86/intel: fix event update for auto-reload

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: <20171219185810.d4yqz6qwqmx3f5o5@hirez.programming.kicks-ass.net>
Date:   Tue, 19 Dec 2017 19:58:10 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     kan.liang@...ux.intel.com
Cc:     mingo@...hat.com, acme@...nel.org, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, jolsa@...hat.com, eranian@...gle.com,
        ak@...ux.intel.com
Subject: Re: [PATCH 2/4] perf/x86/intel: fix event update for auto-reload

On Mon, Dec 18, 2017 at 03:34:49AM -0800, kan.liang@...ux.intel.com wrote:
>  arch/x86/events/core.c     | 14 ++++++++++++++
>  arch/x86/events/intel/ds.c |  8 +++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 35552ea..f74e21d 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -100,6 +100,20 @@ u64 x86_perf_event_update(struct perf_event *event,
>  	 * of the count.
>  	 */
>  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
> +
> +	/*
> +	 * Take auto-reload into account
> +	 * For the auto-reload before the last time, it went through the
> +	 * whole period (reload_val) every time.
> +	 * Just simply add period * times to the event.
> +	 *
> +	 * For the last load, the elapsed delta (event-)time need to be
> +	 * corrected by adding the period. Because the start point is -period.
> +	 */
> +	if (reload_times > 0) {
> +		delta += (reload_val << shift);
> +		local64_add(reload_val * (reload_times - 1), &event->count);
> +	}
>  	delta >>= shift;
>  
>  	local64_add(delta, &event->count);
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 0b693b7..f0f6026 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1256,11 +1256,17 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
>  				   void *base, void *top,
>  				   int bit, int count)
>  {
> +	struct hw_perf_event *hwc = &event->hw;
>  	struct perf_sample_data data;
>  	struct pt_regs regs;
>  	void *at = get_next_pebs_record_by_bit(base, top, bit);
>  
> -	if (!intel_pmu_save_and_restart(event, 0, 0) &&
> +	/*
> +	 * Now, auto-reload is only enabled in fixed period mode.
> +	 * The reload value is always hwc->sample_period.
> +	 * May need to change it, if auto-reload is enabled in freq mode later.
> +	 */
> +	if (!intel_pmu_save_and_restart(event, hwc->sample_period, count - 1) &&
>  	    !(event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD))
>  		return;
>  

This all looks very wrong... In auto reload we should never call
intel_pmu_save_and_restore() in the first place I think.

Things like x86_perf_event_update() and x86_perf_event_set_period()
simply _cannot_ do the right thing when we auto reload the counter.