linux-kernel - Re: [PATCH 0/4] TopDown metrics support for Ice Lake (perf tool)

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] [day] [month] [year] [list]

Date:   Thu, 27 Aug 2020 08:39:39 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>, acme@...nel.org
Cc:     peterz@...radead.org, mingo@...hat.com, namhyung@...nel.org,
        linux-kernel@...r.kernel.org, eranian@...gle.com,
        ak@...ux.intel.com
Subject: Re: [PATCH 0/4] TopDown metrics support for Ice Lake (perf tool)



On 8/26/2020 11:54 AM, Jiri Olsa wrote:
> On Thu, Aug 20, 2020 at 09:45:28AM -0700, kan.liang@...ux.intel.com wrote:
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> The kernel patches have been merged into the tip's perf/core branch.
>> The patch set is on top of commit 2cb5383b30d4 ("perf/x86/intel: Support
>> per-thread RDPMC TopDown metrics") of the tip's perf/core branch.
>>
>> The changes for the perf tool include:
>> - Extend --topdown option to support per thread TopDown metrics
>> - Support sample-read topdown metric group
>> - Add a complete document for the TopDown usage.
>>
>> Ice Lake has support for measuring the level 1 TopDown metrics
>> directly in hardware. This is implemented by an additional METRICS
>> register, and a new Fixed Counter 3 that measures pipeline SLOTS.
>>
>> New in Icelake
>> - Do not require generic counters. This allows to collect TopDown always
>>    in addition to other events.
>> - Measuring TopDown per thread/process instead of only per core
>>
>> For the Ice Lake implementation of performance metrics, the values in
>> PERF_METRICS MSR are derived from fixed counter 3. Software should start
>> both registers, PERF_METRICS and fixed counter 3, from zero.
>> Additionally, software is recommended to periodically clear both
>> registers in order to maintain accurate measurements. The latter is
>> required for certain scenarios that involve sampling metrics at high
>> rates. Software should always write fixed counter 3 before write to
>> PERF_METRICS.
>>
>> IA32_PERF_GLOBAL_STATUS. OVF_PERF_METRICS[48]: If this bit is set,
>> it indicates that some PERF_METRICS-related counter has overflowed and
>> a PMI is triggered. Software has to synchronize, e.g. re-start,
>> PERF_METRICS as well as fixed counter 3. Otherwise, PERF_METRICS may
>> return invalid values.
>>
>> Limitation
>> - To get accurate result and avoid reading the METRICS register multiple
>>    times, the TopDown metrics events and SLOTS event have to be in the
>>    same group.
>> - METRICS and SLOTS registers have to be cleared after each read by SW.
>>    That is to prevent the lose of precision.
>> - Cannot do sampling read SLOTS and TopDown metric events
>>
>> Please refer SDM Vol3, 18.3.9.3 Performance Metrics for the details of
>> TopDown metrics.
>>
>> Andi Kleen (2):
>>    perf stat: Support new per thread TopDown metrics
>>    perf, tools: Add documentation for topdown metrics
>>
>> Kan Liang (2):
>>    perf tools: Rename group to topdown
>>    perf record: Support sample-read topdown metric group
> 
> I don't have Ice lake to actualy check, but it looks good
> 
> Acked-by: Jiri Olsa <jolsa@...hat.com>
> 


Thanks Jirka for the review.

Hi Arnaldo,

Andi has a comment regarding a grammar error in the printf message. I 
once plan to apply a fix in the next version. I'm not sure whether you 
will have more comments for this patch set.
If yes, I will wait for the comments.
If not, should I send a V2 patch set with this fixed?

On 8/20/2020 4:05 PM, Andi Kleen wrote:
 >> +				fprintf(stat_config.output,
 >> +					"Topdown accuracy may decreases when measuring long period.\n"
 > "may decrease" ... "long periods".
 >
 > (s needs to move to the end)


Thanks,
Kan