Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa [off-list ref] wrote:

On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:

quoted

On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers [off-list ref] wrote:

quoted

On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa [off-list ref] wrote:

quoted

On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote:

quoted

On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote:

quoted

From: Stephane Eranian <redacted>

Before:
$ perf record -c 10000 --pfm-events=cycles:period=77777

Would yield a cycles event with period=10000, instead of 77777.

This was due to an ordering issue between libpfm4 parsing
the event string and perf record initializing the event.

This patch fixes the problem by preventing override for
events with attr->sample_period != 0 by the time
perf_evsel__config() is invoked. This seems to have been the
intent of the author.

Signed-off-by: Stephane Eranian <redacted>
Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 811f538f7d77..8afc24e2ec52 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c

@@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
     * We default some events to have a default interval. But keep
     * it a weak assumption overridable by the user.
     */
-   if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
-                                opts->user_interval != ULLONG_MAX)) {
+   if (!attr->sample_period) {

I was wondering why this wouldn't break record/top
but we take care of the via record_opts__config

as long as 'perf test attr' works it looks ok to me

hum ;-)

[jolsa@krava perf]$ sudo ./perf test 17 -v
17: Setup struct perf_event_attr                          :
...
running './tests/attr/test-record-C0'
expected sample_period=4000, got 3000
FAILED './tests/attr/test-record-C0' - match failure

I'm not able to reproduce this. Do you have a build configuration or
something else to look at? The test doesn't seem obviously connected
with this patch.

Thanks,
Ian

Jiri, any update? Thanks,

sorry, I rebased and ran it again and it passes for me now,
so it got fixed along the way

No worries, thanks for the update! It'd be nice to land this and the
other libpfm fixes.

Ian

jirka