* [PATCH] perf/x86/intel: Fix debug_store reset field for freq events
@ 2017-07-14 16:35 Jiri Olsa
2017-07-14 17:22 ` Andi Kleen
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jiri Olsa @ 2017-07-14 16:35 UTC (permalink / raw
To: Peter Zijlstra
Cc: lkml, Ingo Molnar, Andi Kleen, Alexander Shishkin, Kan Liang
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 <jolsa@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-14 16:35 [PATCH] perf/x86/intel: Fix debug_store reset field for freq events Jiri Olsa
@ 2017-07-14 17:22 ` Andi Kleen
2017-07-17 7:38 ` Jiri Olsa
2017-07-17 11:37 ` Jiri Olsa
2017-07-18 10:44 ` [tip:perf/urgent] " tip-bot for Jiri Olsa
2017-07-18 12:16 ` tip-bot for Jiri Olsa
2 siblings, 2 replies; 7+ messages in thread
From: Andi Kleen @ 2017-07-14 17:22 UTC (permalink / raw
To: Jiri Olsa
Cc: Peter Zijlstra, lkml, Ingo Molnar, Alexander Shishkin, Kan Liang
Jiri Olsa <jolsa@kernel.org> writes:
>
> Setting the reset field to 0 for freq events.
Looks good to me.
Reviewed-by: Andi Kleen <ak@linux.intel.com>
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-14 17:22 ` Andi Kleen
@ 2017-07-17 7:38 ` Jiri Olsa
2017-07-17 11:37 ` Jiri Olsa
1 sibling, 0 replies; 7+ messages in thread
From: Jiri Olsa @ 2017-07-17 7:38 UTC (permalink / raw
To: Andi Kleen
Cc: Jiri Olsa, Peter Zijlstra, lkml, Ingo Molnar, Alexander Shishkin,
Kan Liang
On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa <jolsa@kernel.org> writes:
> >
> > Setting the reset field to 0 for freq events.
>
> Looks good to me.
>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-14 17:22 ` Andi Kleen
2017-07-17 7:38 ` Jiri Olsa
@ 2017-07-17 11:37 ` Jiri Olsa
2017-07-17 18:07 ` Andi Kleen
1 sibling, 1 reply; 7+ messages in thread
From: Jiri Olsa @ 2017-07-17 11:37 UTC (permalink / raw
To: Andi Kleen
Cc: Jiri Olsa, Peter Zijlstra, lkml, Ingo Molnar, Alexander Shishkin,
Kan Liang
On Fri, Jul 14, 2017 at 10:22:49AM -0700, Andi Kleen wrote:
> Jiri Olsa <jolsa@kernel.org> writes:
> >
> > Setting the reset field to 0 for freq events.
>
> Looks good to me.
>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-17 11:37 ` Jiri Olsa
@ 2017-07-17 18:07 ` Andi Kleen
0 siblings, 0 replies; 7+ messages in thread
From: Andi Kleen @ 2017-07-17 18:07 UTC (permalink / raw
To: Jiri Olsa
Cc: Andi Kleen, Jiri Olsa, Peter Zijlstra, lkml, Ingo Molnar,
Alexander Shishkin, Kan Liang
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 <jolsa@kernel.org> writes:
> > >
> > > Setting the reset field to 0 for freq events.
> >
> > Looks good to me.
> >
> > Reviewed-by: Andi Kleen <ak@linux.intel.com>
> >
> > 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 xxxx' and '--no-timestamp'
PERF_SAMPLE_TIME works with Skylake/goldmont, but yes forgot
the -c.
-Andi
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip:perf/urgent] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-14 16:35 [PATCH] perf/x86/intel: Fix debug_store reset field for freq events Jiri Olsa
2017-07-14 17:22 ` Andi Kleen
@ 2017-07-18 10:44 ` tip-bot for Jiri Olsa
2017-07-18 12:16 ` tip-bot for Jiri Olsa
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Jiri Olsa @ 2017-07-18 10:44 UTC (permalink / raw
To: linux-tip-commits
Cc: kan.liang, tglx, torvalds, a.p.zijlstra, mingo, jolsa, hpa,
linux-kernel, peterz, alexander.shishkin
Commit-ID: b32fd2f3f667c8a94f2f6bc2fb88a607be8f3229
Gitweb: http://git.kernel.org/tip/b32fd2f3f667c8a94f2f6bc2fb88a607be8f3229
Author: Jiri Olsa <jolsa@kernel.org>
AuthorDate: Fri, 14 Jul 2017 18:35:51 +0200
Committer: Ingo Molnar <mingo@kernel.org>
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 <jolsa@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170714163551.19459-1-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
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 2ca4d2d..6dc8a59 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -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;
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [tip:perf/urgent] perf/x86/intel: Fix debug_store reset field for freq events
2017-07-14 16:35 [PATCH] perf/x86/intel: Fix debug_store reset field for freq events Jiri Olsa
2017-07-14 17:22 ` Andi Kleen
2017-07-18 10:44 ` [tip:perf/urgent] " tip-bot for Jiri Olsa
@ 2017-07-18 12:16 ` tip-bot for Jiri Olsa
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Jiri Olsa @ 2017-07-18 12:16 UTC (permalink / raw
To: linux-tip-commits
Cc: kan.liang, jolsa, mingo, alexander.shishkin, torvalds,
linux-kernel, a.p.zijlstra, tglx, hpa, peterz
Commit-ID: dc853e26f73e903e0c87e24f2695b5dcf33b3bc1
Gitweb: http://git.kernel.org/tip/dc853e26f73e903e0c87e24f2695b5dcf33b3bc1
Author: Jiri Olsa <jolsa@kernel.org>
AuthorDate: Fri, 14 Jul 2017 18:35:51 +0200
Committer: Ingo Molnar <mingo@kernel.org>
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 <jolsa@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170714163551.19459-1-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
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 2ca4d2d..6dc8a59 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -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;
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-18 12:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-14 16:35 [PATCH] perf/x86/intel: Fix debug_store reset field for freq events Jiri Olsa
2017-07-14 17:22 ` Andi Kleen
2017-07-17 7:38 ` Jiri Olsa
2017-07-17 11:37 ` Jiri Olsa
2017-07-17 18:07 ` Andi Kleen
2017-07-18 10:44 ` [tip:perf/urgent] " tip-bot for Jiri Olsa
2017-07-18 12:16 ` tip-bot for Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.