Message ID | 20200728085734.609930-3-irogers@google.com |
---|---|
State | Not Applicable |
Delegated to: | BPF Maintainers |
Headers | show |
Series | Fixes for setting event freq/periods | expand |
On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > From: Stephane Eranian <eranian@google.com> > > 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 <eranian@google.com> > 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 Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka
On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > From: Stephane Eranian <eranian@google.com> > > > > 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 <eranian@google.com> > > 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 jirka > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > thanks, > jirka
Em Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers escreveu: > From: Stephane Eranian <eranian@google.com> > > Before: > $ perf record -c 10000 --pfm-events=cycles:period=77777 > > Would yield a cycles event with period=10000, instead of 77777. I tried the equivalent without libpfm and it works: $ perf record -c 10000 -e cycles/period=20000/ sleep 1 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.024 MB perf.data (23 samples) ] $ perf evlist -v cycles/period=20000/u: size: 120, { sample_period, sample_freq }: 20000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_kernel: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 $ > 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 <eranian@google.com> > 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) { > if (opts->freq) { > attr->freq = 1; > attr->sample_freq = opts->freq; > -- > 2.28.0.163.g6104cc2f0b6-goog >
On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > From: Stephane Eranian <eranian@google.com> > > > > > > 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 <eranian@google.com> > > > 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 > jirka > > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > > > thanks, > > jirka >
On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > From: Stephane Eranian <eranian@google.com> > > > > > > > > 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 <eranian@google.com> > > > > 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, Ian > > jirka > > > > > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > > > > > thanks, > > > jirka > >
On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > From: Stephane Eranian <eranian@google.com> > > > > > > > > > > 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 <eranian@google.com> > > > > > 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 jirka
On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > > From: Stephane Eranian <eranian@google.com> > > > > > > > > > > > > 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 <eranian@google.com> > > > > > > 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 >
Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu: > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > [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. > > > 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. I applied it and it generated this regression: FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure I'll look at the other patches that are pending in this regard to see what needs to be squashed so that we don't break bisect. - Arnaldo
Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu: > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > > [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. > > > > > 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. > > I applied it and it generated this regression: > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > I'll look at the other patches that are pending in this regard to see > what needs to be squashed so that we don't break bisect. So, more context: running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' expected exclude_hv=0, got 1 FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure test child finished with -1 ---- end ---- Setup struct perf_event_attr: FAILED! [root@five ~]# Ian, can you take a look at this? - Arnaldo
Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu: > > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > > > [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. > > > > > > > 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. > > > > I applied it and it generated this regression: > > > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > > > I'll look at the other patches that are pending in this regard to see > > what needs to be squashed so that we don't break bisect. > > So, more context: > > running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' > expected exclude_hv=0, got 1 > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > test child finished with -1 > ---- end ---- > Setup struct perf_event_attr: FAILED! > [root@five ~]# > > Ian, can you take a look at this? Further tests I've performed: Committer testing: Not linking with libpfm: # ldd ~/bin/perf | grep libpfm # Before: # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ] # perf evlist -v cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 # After: # # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ] # perf evlist -v cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 # Linking with libpfm: # ldd ~/bin/perf | grep libpfm libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000) # # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ] # perf evlist -v cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 # After: # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ] # perf evlist -v cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 #
On Fri, Sep 4, 2020 at 11:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu: > > > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > > > > [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. > > > > > > > > > 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. > > > > > > I applied it and it generated this regression: > > > > > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > > > > > I'll look at the other patches that are pending in this regard to see > > > what needs to be squashed so that we don't break bisect. > > > > So, more context: > > > > running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' > > expected exclude_hv=0, got 1 > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > test child finished with -1 > > ---- end ---- > > Setup struct perf_event_attr: FAILED! > > [root@five ~]# > > > > Ian, can you take a look at this? > > Further tests I've performed: > > Committer testing: > > Not linking with libpfm: > > # ldd ~/bin/perf | grep libpfm > # > > Before: > > # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ] > # perf evlist -v > cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 > # > > After: > > # > # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ] > # perf evlist -v > cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 > # > > Linking with libpfm: > > # ldd ~/bin/perf | grep libpfm > libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000) > # > > # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ] > # perf evlist -v > cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > # > > After: > > # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ] > # perf evlist -v > cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > # > Hi Arnaldo, I've been trying to reproduce the test failure you mention and I've not been able to. This follow up e-mail seems to show things working as intended. Did the issue resolve itself? Thanks, Ian
On Fri, Sep 11, 2020 at 3:34 PM Ian Rogers <irogers@google.com> wrote: > > On Fri, Sep 4, 2020 at 11:51 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu: > > > > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote: > > > > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote: > > > > > > > > On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote: > > > > > > > > > > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote: > > > > > > > > > [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. > > > > > > > > > > > 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. > > > > > > > > I applied it and it generated this regression: > > > > > > > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > > > > > > > I'll look at the other patches that are pending in this regard to see > > > > what needs to be squashed so that we don't break bisect. > > > > > > So, more context: > > > > > > running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' > > > expected exclude_hv=0, got 1 > > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure > > > test child finished with -1 > > > ---- end ---- > > > Setup struct perf_event_attr: FAILED! > > > [root@five ~]# > > > > > > Ian, can you take a look at this? > > > > Further tests I've performed: > > > > Committer testing: > > > > Not linking with libpfm: > > > > # ldd ~/bin/perf | grep libpfm > > # > > > > Before: > > > > # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 > > [ perf record: Woken up 1 times to write data ] > > [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ] > > # perf evlist -v > > cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > > instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 > > # > > > > After: > > > > # > > # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001 > > [ perf record: Woken up 1 times to write data ] > > [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ] > > # perf evlist -v > > cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > > instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 > > # > > > > Linking with libpfm: > > > > # ldd ~/bin/perf | grep libpfm > > libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000) > > # > > > > # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 > > [ perf record: Woken up 1 times to write data ] > > [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ] > > # perf evlist -v > > cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > > # > > > > After: > > > > # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1 > > [ perf record: Woken up 1 times to write data ] > > [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ] > > # perf evlist -v > > cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1 > > # > > > Hi Arnaldo, > > I've been trying to reproduce the test failure you mention and I've > not been able to. This follow up e-mail seems to show things working > as intended. Did the issue resolve itself? It looks like the test to ensure the period for pfm events worked: https://lore.kernel.org/lkml/20200728124539.GB40195@kernel.org/ has been applied without the fixes in patches 1 and 2. I've resent the patches, hopefully addressing all feedback. https://lore.kernel.org/lkml/20200912025655.1337192-1-irogers@google.com/ Thanks, Ian
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) { if (opts->freq) { attr->freq = 1; attr->sample_freq = opts->freq;