Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Andi Kleen]

On Mon, Jul 17, 2017 at 01:37:58PM +0200, Jiri Olsa wrote:
> On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> > Jiri Olsa  writes:
> > >
> > > Setting the reset field to 0 for freq events.
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Andi Kleen 
> > 
> > BTW I suspect there's a related bug that
> > 
> > perf record -e '{cycles:pp,branches}:S' ..
> > 
> > would enable multi record PEBS, even though it shouldn't because
> > we need the PMI to read the other events.
> 
> there's PERF_SAMPLE_READ om cycles's sample_type for this example
> so it won't pass the x86_pmu::free_running_flags filter

Good thanks for checking.

> 
> also PERF_SAMPLE_TIME and PERF_SAMPLE_PERIOD will be set
> in your example which will prevent that, but those
> could be unset via record's '-c ' and '--no-timestamp'

PERF_SAMPLE_TIME works with Skylake/goldmont, but yes forgot 
the -c.

-Andi

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Andi Kleen]

On Mon, Jul 17, 2017 at 01:37:58PM +0200, Jiri Olsa wrote:
> On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> > Jiri Olsa  writes:
> > >
> > > Setting the reset field to 0 for freq events.
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Andi Kleen 
> > 
> > BTW I suspect there's a related bug that
> > 
> > perf record -e '{cycles:pp,branches}:S' ..
> > 
> > would enable multi record PEBS, even though it shouldn't because
> > we need the PMI to read the other events.
> 
> there's PERF_SAMPLE_READ om cycles's sample_type for this example
> so it won't pass the x86_pmu::free_running_flags filter

Good thanks for checking.

> 
> also PERF_SAMPLE_TIME and PERF_SAMPLE_PERIOD will be set
> in your example which will prevent that, but those
> could be unset via record's '-c ' and '--no-timestamp'

PERF_SAMPLE_TIME works with Skylake/goldmont, but yes forgot 
the -c.

-Andi

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa  writes:
> >
> > Setting the reset field to 0 for freq events.
> 
> Looks good to me.
> 
> Reviewed-by: Andi Kleen 
> 
> BTW I suspect there's a related bug that
> 
> perf record -e '{cycles:pp,branches}:S' ..
> 
> would enable multi record PEBS, even though it shouldn't because
> we need the PMI to read the other events.

there's PERF_SAMPLE_READ om cycles's sample_type for this example
so it won't pass the x86_pmu::free_running_flags filter

also PERF_SAMPLE_TIME and PERF_SAMPLE_PERIOD will be set
in your example which will prevent that, but those
could be unset via record's '-c ' and '--no-timestamp'

jirka

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa  writes:
> >
> > Setting the reset field to 0 for freq events.
> 
> Looks good to me.
> 
> Reviewed-by: Andi Kleen 
> 
> BTW I suspect there's a related bug that
> 
> perf record -e '{cycles:pp,branches}:S' ..
> 
> would enable multi record PEBS, even though it shouldn't because
> we need the PMI to read the other events.

there's PERF_SAMPLE_READ om cycles's sample_type for this example
so it won't pass the x86_pmu::free_running_flags filter

also PERF_SAMPLE_TIME and PERF_SAMPLE_PERIOD will be set
in your example which will prevent that, but those
could be unset via record's '-c ' and '--no-timestamp'

jirka

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa  writes:
> >
> > Setting the reset field to 0 for freq events.
> 
> Looks good to me.
> 
> Reviewed-by: Andi Kleen 
> 
> BTW I suspect there's a related bug that
> 
> perf record -e '{cycles:pp,branches}:S' ..
> 
> would enable multi record PEBS, even though it shouldn't because
> we need the PMI to read the other events.

thanks, I'll check on that

jirka

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa  writes:
> >
> > Setting the reset field to 0 for freq events.
> 
> Looks good to me.
> 
> Reviewed-by: Andi Kleen 
> 
> BTW I suspect there's a related bug that
> 
> perf record -e '{cycles:pp,branches}:S' ..
> 
> would enable multi record PEBS, even though it shouldn't because
> we need the PMI to read the other events.

thanks, I'll check on that

jirka

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Andi Kleen]

Jiri Olsa  writes:
>
> Setting the reset field to 0 for freq events.

Looks good to me.

Reviewed-by: Andi Kleen 

BTW I suspect there's a related bug that

perf record -e '{cycles:pp,branches}:S' ..

would enable multi record PEBS, even though it shouldn't because
we need the PMI to read the other events.

-Andi

Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Andi Kleen]

Jiri Olsa  writes:
>
> Setting the reset field to 0 for freq events.

Looks good to me.

Reviewed-by: Andi Kleen 

BTW I suspect there's a related bug that

perf record -e '{cycles:pp,branches}:S' ..

would enable multi record PEBS, even though it shouldn't because
we need the PMI to read the other events.

-Andi

[PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

There's a bug in PEBs event enabling code, that prevents PEBS
freq events to work properly after non freq PEBS event was run.

freq events - perf_event_attr::freq set
  -F  option of perf record

PEBS events - perf_event_attr::precise_ip > 0
  default for perf record

Like in following example with cpu 0 busy, we expect ~1 samples
for following perf tool run:

  # perf record -F 1 -C 0 sleep 1
  [ perf record: Woken up 2 times to write data ]
  [ perf record: Captured and wrote 0.640 MB perf.data (10031 samples) ]

Everything's fine, but once we run non freq PEBS event like:

  # perf record -c 1 -C 0 sleep 1
  [ perf record: Woken up 4 times to write data ]
  [ perf record: Captured and wrote 1.053 MB perf.data (20061 samples) ]

the freq events start to fail like this:

  # perf record -F 1 -C 0 sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.185 MB perf.data (40 samples) ]

The issue is in non freq PEBs event initialization of debug_store reset
field, which value is used to auto-reload the counter value after PEBS
event drain. This value is not being used for PEBS freq events, but once
we run non freq event it stays in debug_store data and screws the
sample_freq counting for PEBS freq events.

Setting the reset field to 0 for freq events.

Signed-off-by: Jiri Olsa 
---
 arch/x86/events/intel/ds.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index c6d23ffe422d..2244bd8c09b1 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -889,6 +889,8 @@ void intel_pmu_pebs_enable(struct perf_event *event)
if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD) {
ds->pebs_event_reset[hwc->idx] =
(u64)(-hwc->sample_period) & x86_pmu.cntval_mask;
+   } else {
+   ds->pebs_event_reset[hwc->idx] = 0;
}
 }
 
-- 
2.9.4

[PATCH] perf/x86/intel: Fix debug_store reset field for freq events

[Jiri Olsa]

There's a bug in PEBs event enabling code, that prevents PEBS
freq events to work properly after non freq PEBS event was run.

freq events - perf_event_attr::freq set
  -F  option of perf record

PEBS events - perf_event_attr::precise_ip > 0
  default for perf record

Like in following example with cpu 0 busy, we expect ~1 samples
for following perf tool run:

  # perf record -F 1 -C 0 sleep 1
  [ perf record: Woken up 2 times to write data ]
  [ perf record: Captured and wrote 0.640 MB perf.data (10031 samples) ]

Everything's fine, but once we run non freq PEBS event like:

  # perf record -c 1 -C 0 sleep 1
  [ perf record: Woken up 4 times to write data ]
  [ perf record: Captured and wrote 1.053 MB perf.data (20061 samples) ]

the freq events start to fail like this:

  # perf record -F 1 -C 0 sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.185 MB perf.data (40 samples) ]

The issue is in non freq PEBs event initialization of debug_store reset
field, which value is used to auto-reload the counter value after PEBS
event drain. This value is not being used for PEBS freq events, but once
we run non freq event it stays in debug_store data and screws the
sample_freq counting for PEBS freq events.

Setting the reset field to 0 for freq events.

Signed-off-by: Jiri Olsa 
---
 arch/x86/events/intel/ds.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index c6d23ffe422d..2244bd8c09b1 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -889,6 +889,8 @@ void intel_pmu_pebs_enable(struct perf_event *event)
if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD) {
ds->pebs_event_reset[hwc->idx] =
(u64)(-hwc->sample_period) & x86_pmu.cntval_mask;
+   } else {
+   ds->pebs_event_reset[hwc->idx] = 0;
}
 }
 
-- 
2.9.4