linux-kernel - Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware TopDown metrics

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: <20200728123240.GJ10769@hirez.programming.kicks-ass.net>
Date:   Tue, 28 Jul 2020 14:32:40 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     acme@...hat.com, mingo@...nel.org, linux-kernel@...r.kernel.org,
        jolsa@...nel.org, eranian@...gle.com,
        alexander.shishkin@...ux.intel.com, ak@...ux.intel.com,
        like.xu@...ux.intel.com
Subject: Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware
 TopDown metrics

On Fri, Jul 24, 2020 at 03:10:52PM -0400, Liang, Kan wrote:

> > > > Per the SIBLING patch this then wants to be:
> > > > 
> > > >             if (!is_slots_event(leader))
> > > >                 return -EINVAL;
> > > > 
> > > >             event->event_caps |= PERF_EV_CAP_SIBLING.
> > > >             /*
> > > >              * Only once we have a METRICs sibling to we
> > > >              * need TopDown magic.
> > > >              */
> > > >             leader->hw.flags |= PERF_X86_EVENT_TOPDOWN;
> 
> Since we only set the flag for the SLOTS event now, the V7 patch will treat
> the metric events as normal events, which trigger an error.

Damn, that was a silly oversight on my part.

> I think we don't need the PERF_X86_EVENT_TOPDOWN flag anymore.
> If it's a non-sampling slots event, apply the special function.
> If it's a metric event, do nothing.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 0f3d01562ded..02dfee0b6615 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -73,10 +73,10 @@ u64 x86_perf_event_update(struct perf_event *event)
>  	u64 prev_raw_count, new_raw_count;
>  	u64 delta;
> 
> -	if (unlikely(!hwc->event_base))
> +	if (unlikely(!hwc->event_base || is_metric_event(event)))
>  		return 0;
> 
> -	if (unlikely(is_topdown_count(event)) && x86_pmu.update_topdown_event)
> +	if (unlikely(is_slots_count(event)) && x86_pmu.update_topdown_event)
>  		return x86_pmu.update_topdown_event(event);
> 
>  	/*
> @@ -1280,11 +1280,10 @@ int x86_perf_event_set_period(struct perf_event
> *event)
>  	s64 period = hwc->sample_period;
>  	int ret = 0, idx = hwc->idx;
> 
> -	if (unlikely(!hwc->event_base))
> +	if (unlikely(!hwc->event_base || is_metric_event(event)))
>  		return 0;
> 
> -	if (unlikely(is_topdown_count(event)) &&
> -	    x86_pmu.set_topdown_event_period)
> +	if (unlikely(is_slots_count(event)) && x86_pmu.set_topdown_event_period)
>  		return x86_pmu.set_topdown_event_period(event);
> 
>  	/*

This; I don't like that much, it adds even more conditions to fairly hot
code.

I was even considering adding a static_branch for
x86_pmu.intel_cap.perf_metrics.

Anyway, let me fix this.