Skip to contentSkip to conversation

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

PATCH In review Jiri Olsa 4 participants 7 messages Jul 14, 2017 – Jul 18, 2017

Subsystems (2)
x86 perf events
lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

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

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 <freq> 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 ~10000 samples for following perf tool run:

  # perf record -F 10000 -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 10000 -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 10000 -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 <[email protected]>
---
 arch/x86/events/intel/ds.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/intel/ds.c +2 -0
index c6d23ffe422d..2244bd8c09b1 100644
@@ -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;
 	}
 }
Signature · 1 line
2.9.4
Reviewed-by on v1 1/1
#1 lore raw
Quoted 2 lines · Jiri Olsa

Setting the reset field to 0 for freq events.

Looks good to me.

Reviewed-by Andi Kleen <[email protected]>

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

#2 lore raw
Quoted 6 lines · Andi Kleen
Quoted 2 lines · Jiri Olsa

Setting the reset field to 0 for freq events.

Looks good to me.

Quoted 8 lines
Reviewed-by Andi Kleen <[email protected]>

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

#3 lore raw
In reply to Andi Kleen
Quoted 6 lines · Andi Kleen
Quoted 2 lines · Jiri Olsa

Setting the reset field to 0 for freq events.

Looks good to me.

Quoted 8 lines
Reviewed-by Andi Kleen <[email protected]>

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 xxxx' and '--no-timestamp'

jirka

#4 lore raw
Quoted 11 lines · Jiri Olsa
Quoted 10 lines · Andi Kleen
Quoted 2 lines · Jiri Olsa

Setting the reset field to 0 for freq events.

Looks good to me.

Reviewed-by Andi Kleen <[email protected]>

BTW I suspect there's a related bug that

Quoted 7 lines
Quoted 4 lines

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.

Quoted 4 lines

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 xxxx' and '--no-timestamp'

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

-Andi

merge bot
#5 lore raw
In reply to Jiri Olsa
perf/x86/intel: Fix debug_store reset field for freq events
Commit-ID:  b32fd2f3f667c8a94f2f6bc2fb88a607be8f3229
Gitweb:     http://git.kernel.org/tip/b32fd2f3f667c8a94f2f6bc2fb88a607be8f3229
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 14 Jul 2017 18:35:51 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 18 Jul 2017 11:06:12 +0200

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

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 <freq> 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 ~10000 samples for following perf tool run:

  # perf record -F 10000 -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 10000 -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 10000 -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 <[email protected]>
Acked-by Peter Zijlstra (Intel) <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/events/intel/ds.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/intel/ds.c +2 -0
index 2ca4d2d..6dc8a59 100644
@@ -895,6 +895,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;
 	}
 }
merge bot
#6 lore raw
In reply to Jiri Olsa
perf/x86/intel: Fix debug_store reset field for freq events
Commit-ID:  dc853e26f73e903e0c87e24f2695b5dcf33b3bc1
Gitweb:     http://git.kernel.org/tip/dc853e26f73e903e0c87e24f2695b5dcf33b3bc1
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 14 Jul 2017 18:35:51 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 18 Jul 2017 14:13:41 +0200

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

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 <freq> 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 ~10000 samples for following perf tool run:

  # perf record -F 10000 -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 10000 -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 10000 -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 <[email protected]>
Acked-by Peter Zijlstra (Intel) <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/events/intel/ds.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/intel/ds.c +2 -0
index 2ca4d2d..6dc8a59 100644
@@ -895,6 +895,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;
 	}
 }