diff mbox series

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

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

Commit Message

Ian Rogers July 28, 2020, 8:57 a.m. UTC 
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(-)

Comments

Jiri Olsa July 28, 2020, 3:59 p.m. UTC | #1 
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
Jiri Olsa July 28, 2020, 4:09 p.m. UTC | #2 
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
Arnaldo Carvalho de Melo July 29, 2020, 6:54 p.m. UTC | #3 
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
>
Ian Rogers July 29, 2020, 11:24 p.m. UTC | #4 
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
>
Ian Rogers Sept. 4, 2020, 5:41 a.m. UTC | #5 
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
> >
Jiri Olsa Sept. 4, 2020, 4:03 p.m. UTC | #6 
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
Ian Rogers Sept. 4, 2020, 4:22 p.m. UTC | #7 
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
>
Arnaldo Carvalho de Melo Sept. 4, 2020, 6:48 p.m. UTC | #8 
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
Arnaldo Carvalho de Melo Sept. 4, 2020, 6:50 p.m. UTC | #9 
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
Arnaldo Carvalho de Melo Sept. 4, 2020, 6:51 p.m. UTC | #10 
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
      #
Ian Rogers Sept. 11, 2020, 10:34 p.m. UTC | #11 
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
Ian Rogers Sept. 12, 2020, 3:02 a.m. UTC | #12 
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 mbox series

Patch

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;