bugs fix for large PEBS mmap read and rdpmc read
Subsystems (2)
v3 cover bugs fix for large PEBS mmap read and rdpmc read
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU in pmu::read() path for large PEBS. Handled the corner case when reload_times == 0.
- Modified the parameter of intel_pmu_save_and_restart_reload() Discarded local64_cmpxchg
- Added fixes tag
- Added WARN to handle reload_times == 0 || reload_val == 0
Changes since V1:
- Check PERF_X86_EVENT_AUTO_RELOAD before call intel_pmu_save_and_restore()
- Introduce a special purpose intel_pmu_save_and_restart() just for AUTO_RELOAD.
- New patch to disable userspace RDPMC usage if large PEBS is enabled.
------
There is a bug when mmap read event->count with large PEBS enabled. Here is an example.
#./read_count
0x71f0
0x122c0
0x1000000001c54
0x100000001257d
0x200000000bdc5
The bug is caused by two issues.
- In x86_perf_event_update, the calculation of event->count does not take the auto-reload values into account.
- In x86_pmu_read, it doesn't count the undrained values in large PEBS buffers.
The first issue was introduced with the auto-reload mechanism enabled since commit 851559e35fd5 ("perf/x86/intel: Use the PEBS auto reload mechanism when possible")
Patch 1 fixed the issue in x86_perf_event_update.
The second issue was introduced since commit b8241d20699e ("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")
Patch 2-4 fixed the issue in x86_pmu_read.
Besides the two issues, the userspace RDPMC usage is broken for large PEBS as well.
The RDPMC issue was also introduced since commit b8241d20699e ("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")
Patch 5 fixed the RDPMC issue.
The source code of read_count is as below.
struct cpu {
int fd;
struct perf_event_mmap_page *buf;
};int perf_open(struct cpu *ctx, int cpu)
{
struct perf_event_attr attr = {
.type = PERF_TYPE_HARDWARE,
.size = sizeof(struct perf_event_attr),
.sample_period = 100000,
.config = 0,
.sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID |
PERF_SAMPLE_TIME | PERF_SAMPLE_CPU,
.precise_ip = 3,
.mmap = 1,
.comm = 1,
.task = 1,
.mmap2 = 1,
.sample_id_all = 1,
.comm_exec = 1,
};
ctx->buf = NULL;
ctx->fd = syscall(__NR_perf_event_open, &attr, -1, cpu, -1, 0);
if (ctx->fd < 0) {
perror("perf_event_open");
return -1;
}
return 0;
}void perf_close(struct cpu *ctx)
{
close(ctx->fd);
if (ctx->buf)
munmap(ctx->buf, pagesize);
}int main(int ac, char **av)
{
struct cpu ctx;
u64 count;
perf_open(&ctx, 0);
while (1) {
sleep(5);
if (read(ctx.fd, &count, 8) != 8) {
perror("counter read");
break;
}
printf("0x%llx\n", count);
}
perf_close(&ctx);
}Kan Liang (5):
perf/x86/intel: fix event update for auto-reload perf/x86: introduce read function for x86_pmu perf/x86/intel/ds: introduce read function for large pebs perf/x86/intel: introduce read function for intel_pmu perf/x86: fix: disable userspace RDPMC usage for large PEBS
arch/x86/events/core.c | 5 ++- arch/x86/events/intel/core.c | 9 +++++ arch/x86/events/intel/ds.c | 85 ++++++++++++++++++++++++++++++++++++++++++-- arch/x86/events/perf_event.h | 3 ++ 4 files changed, 99 insertions(+), 3 deletions(-)
Signature · 1 line
2.7.4
PATCH v3 1/5 perf/x86/intel: fix event update for auto-reload
From: Kan Liang <[email protected]>
There is a bug when mmap read event->count with large PEBS enabled. Here is an example.
#./read_count
0x71f0
0x122c0
0x1000000001c54
0x100000001257d
0x200000000bdc5
In fixed period mode, the auto-reload mechanism could be enabled for PEBS events. But the calculation of event->count does not take the auto-reload values into account. Anyone who read the event->count will get wrong result, e.g x86_pmu_read. The calculation of hwc->period_left is wrong either, which will impact the calculation of the period for the first record in PEBS multiple records.
The issue was introduced with the auto-reload mechanism enabled since commit 851559e35fd5 ("perf/x86/intel: Use the PEBS auto reload mechanism when possible")
Introduce intel_pmu_save_and_restart_reload to calculate the event->count only for auto-reload.
There is a small gap between 'PEBS hardware is armed' and 'the NMI is handled'. Because of the gap, the first record also needs to be specially handled.
The formula to calculate the increments of event->count is as below. The increments = the period for first record +
(reload_times - 1) * reload_val +
the gap
- The 'the period for first record' is the period left from last PMI, which can be got from the previous event value.
- For the second and later records, the period is exactly the reload value. Just need to simply add (reload_times - 1) * reload_val
- Because of the auto-reload, the start point of counting is alwyas (-reload_val). So the calculation of 'the gap' needs to be corrected by adding reload_val.
The period_left needs to do the same adjustment as well.
There is nothing need to do in x86_perf_event_set_period(). Because it is fixed period. The period_left is already adjusted.
when possible")
--- arch/x86/events/intel/ds.c | 69 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 2 deletions(-)
arch/x86/events/intel/ds.c +67 -2
@@ -1303,17 +1303,82 @@ get_next_pebs_record_by_bit(void *base, void *top, int bit)return NULL ; } +/* +* Specific intel_pmu_save_and_restart() for auto-reload. +* It only be called from drain_pebs(). +*/ +static int intel_pmu_save_and_restart_reload (struct perf_event *event, +int reload_times) +{ +struct hw_perf_event *hwc = &event->hw; +int shift =64 - x86_pmu.cntval_bits; +u64 reload_val = hwc->sample_period; +u64 prev_raw_count, new_raw_count; +u64 delta; + +WARN_ON ((reload_times ==0 ) || (reload_val ==0 )); + +/* +* drain_pebs() only happens when the PMU is disabled. +* It doesnot need to specially handle the previous event value. +* The hwc->prev_count will be updated in x86_perf_event_set_period(). +*/ + prev_raw_count =local64_read (&hwc->prev_count); +rdpmcl (hwc->event_base_rdpmc, new_raw_count); + +/* +* Now we have the new raw value and have updated the prev +* timestamp already. We can now calculate the elapsed delta +* (event-)time and add that to the generic event. +* +* Careful, not all hw sign-extends above the physical width +* of the count. +* +* There is a small gap between 'PEBS hardware is armed' and 'the NMI +* is handled'. Because of the gap, the first record also needs to be +* specially handled. +* The formula to calculate the increments of event->count is as below. +* The increments = the period for first record + +* (reload_times - 1) * reload_val + +* the gap +* 'The period for first record' can be got from -prev_raw_count. +* +* 'The gap' = new_raw_count + reload_val. Because the start point of +* counting is always -reload_val for auto-reload. +* +* The period_left needs to do the same adjustment by adding +* reload_val. +*/ + delta = (reload_val << shift) + (new_raw_count << shift) - + (prev_raw_count << shift); + delta >>= shift; + +local64_add (reload_val * (reload_times -1 ), &event->count); +local64_add (delta, &event->count); +local64_sub (delta, &hwc->period_left); + +return x86_perf_event_set_period (event); +} +static void __intel_pmu_pebs_event (struct perf_event *event,struct pt_regs *iregs,void *base,void *top,int bit,int count) { +struct hw_perf_event *hwc = &event->hw;struct perf_sample_data data;struct pt_regs regs;void *at =get_next_pebs_record_by_bit (base, top, bit); -if (!intel_pmu_save_and_restart (event) && - !(event->hw.flags &PERF_X86_EVENT_AUTO_RELOAD )) +if (hwc->flags &PERF_X86_EVENT_AUTO_RELOAD ) { +/* +* Now, auto-reload is only enabled in fixed period mode. +* The reload value is always hwc->sample_period. +* May need to change it, if auto-reload is enabled in +* freq mode later. +*/ +intel_pmu_save_and_restart_reload (event, count); + }else if (!intel_pmu_save_and_restart (event))return ;while (count >1 ) {
Signature · 1 line
2.7.4
Peter Zijlstra
maintainerRe: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload
Quoted 4 lines · On Mon, Jan 29, 2018 at 08:29:29AM -0800, [email protected] wrote:
(quoted diff) +4
-0+/* + * Specific intel_pmu_save_and_restart() for auto-reload. + * It only be called from drain_pebs(). + */
Review
Quoted 10 lines
(quoted diff) +10
-0+static int intel_pmu_save_and_restart_reload(struct perf_event *event, + int reload_times) +{ + struct hw_perf_event *hwc = &event->hw; + int shift = 64 - x86_pmu.cntval_bits; + u64 reload_val = hwc->sample_period; + u64 prev_raw_count, new_raw_count; + u64 delta; + + WARN_ON((reload_times == 0) || (reload_val == 0));
I'm struggling to see why !count is a problem. You can call read() twice without having extra PEBS samples, right?
Which also seems to suggest we have a problem in intel_pmu_drain_pebs*() because those will simply not call us when there aren't any PEBS events in the buffer, even though the count value can have changed. Your patch doesn't appear to address that.
Review
Quoted 5 lines
(quoted diff) +5
-0+ + /* + * drain_pebs() only happens when the PMU is disabled. + * It doesnot need to specially handle the previous event value. + * The hwc->prev_count will be updated in x86_perf_event_set_period().
That's completely buggered. You shouldn't need to call x86_perf_event_set_period() _at_all_.
Review
Quoted 1 line
(quoted diff) +1
-0+ */
WARN_ON(this_cpu_read(cpu_hw_events.enabled));
Quoted 4 lines
(quoted diff) +4
-0+ prev_raw_count = local64_read(&hwc->prev_count); + rdpmcl(hwc->event_base_rdpmc, new_raw_count); + + /*
Review
Quoted 10 lines
(quoted diff) +10
-0+ * Now we have the new raw value and have updated the prev + * timestamp already. We can now calculate the elapsed delta + * (event-)time and add that to the generic event. + * + * Careful, not all hw sign-extends above the physical width + * of the count. + * + * There is a small gap between 'PEBS hardware is armed' and 'the NMI + * is handled'. Because of the gap, the first record also needs to be + * specially handled.
True, but not at all relevant.
Review
Quoted 8 lines
(quoted diff) +8
-0+ * The formula to calculate the increments of event->count is as below. + * The increments = the period for first record + + * (reload_times - 1) * reload_val + + * the gap + * 'The period for first record' can be got from -prev_raw_count. + * + * 'The gap' = new_raw_count + reload_val. Because the start point of + * counting is always -reload_val for auto-reload.
That is still very much confused, what you seem to want is something like:
Since the counter increments a negative counter value and overflows on the sign switch, giving the interval:
[-period, 0]
the difference between two consequtive reads is:
A) value2 - value1;
when no overflows have happened in between,
B) (0 - value1) + (value2 - (-period)); when one overflow happened in between,
C) (0 - value1) + (n - 1) * (period) + (value2 - (-period)); when @n overflows happened in betwee.
Here A) is the obvious difference, B) is the extention to the discrete interval, where the first term is to the top of the interval and the second term is from the bottom of the next interval and 3) the extention to multiple intervals, where the middle term is the whole intervals covered.
An equivalent of C, by reduction, is:
value2 - value1 + n * period
Review
Quoted 11 lines
(quoted diff) +11
-0+ * + * The period_left needs to do the same adjustment by adding + * reload_val. + */ + delta = (reload_val << shift) + (new_raw_count << shift) - + (prev_raw_count << shift); + delta >>= shift; + + local64_add(reload_val * (reload_times - 1), &event->count); + local64_add(delta, &event->count); + local64_sub(delta, &hwc->period_left);
Nobody cares about period_left, since AUTO_RELOAD already limits the periods to what the hardware supports, right?
Review
Quoted 3 lines
(quoted diff) +3
-0+ + return x86_perf_event_set_period(event); +}
With the exception of handling 'empty' buffers, I ended up with the below. Please try again.
---
arch/x86/events/core.c +6 -9
@@ -1156,16 +1156,13 @@ int x86_perf_event_set_period(struct perper_cpu (pmc_prev_left[idx],smp_processor_id ()) = left; -if (!(hwc->flags &PERF_X86_EVENT_AUTO_RELOAD ) || -local64_read (&hwc->prev_count) != (u64)-left) { -/* -* The hw event starts counting from this event offset, -* mark it to be able to extra future deltas: -*/ -local64_set (&hwc->prev_count, (u64)-left); +/* +* The hw event starts counting from this event offset, +* mark it to be able to extra future deltas: +*/ +local64_set (&hwc->prev_count, (u64)-left); -wrmsrl (hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask); - } +wrmsrl (hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);/* * Due to erratum on certan cpu we need
arch/x86/events/intel/ds.c +74 -2
91 lines not on this page · raw message
Re: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload
Quoted 5 lines
With the exception of handling 'empty' buffers, I ended up with the below. Please try again.
There are two small errors. After fixing them, the patch works well.
Quoted 44 lines
---
arch/x86/events/core.c +6
-9@@ -1156,16 +1156,13 @@ int x86_perf_event_set_period(struct perper_cpu (pmc_prev_left[idx],smp_processor_id ()) = left; -if (!(hwc->flags &PERF_X86_EVENT_AUTO_RELOAD ) || -local64_read (&hwc->prev_count) != (u64)-left) { -/* -* The hw event starts counting from this event offset, -* mark it to be able to extra future deltas: -*/ -local64_set (&hwc->prev_count, (u64)-left); +/* +* The hw event starts counting from this event offset, +* mark it to be able to extra future deltas: +*/ +local64_set (&hwc->prev_count, (u64)-left); -wrmsrl (hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask); - } +wrmsrl (hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);/* * Due to erratum on certan cpu we need arch/x86/events/intel/ds.c +12
-0@@ -1306,17 +1306,89 @@ get_next_pebs_record_by_bit(void *base,return NULL ; } +/* +* Special variant of intel_pmu_save_and_restart() for auto-reload. +*/ +static int +intel_pmu_save_and_restart_reload (struct perf_event *event,int count) +{ +struct hw_perf_event *hwc = &event->hw; +int shift =64 - x86_pmu.cntval_bits; +u64 period = hwc->sample_period; +u64 prev_raw_count, new_raw_count; +u64 delta; +
arch/x86/events/intel/ds.c get_next_pebs_record_by_bit(void *base, · line 1330
Quoted 10 lines
arch/x86/events/intel/ds.c +10
-0+WARN_ON (!period); + +/* +* drain_pebs() only happens when the PMU is disabled. +*/ +WARN_ON (this_cpu_read (cpu_hw_events.enabled)); + + prev_raw_count =local64_read (&hwc->prev_count); +rdpmcl (hwc->event_base_rdpmc, new_raw_count); +local64_set (&hwx->prev_count, new_raw_count);
Here is a typo. It should be &hwc->prev_count.
arch/x86/events/intel/ds.c get_next_pebs_record_by_bit(void *base, · line 1337
Quoted 7 lines
arch/x86/events/intel/ds.c +7
-0+ +/* +* Careful, not all hw sign-extends above the physical width +* of the counter. +*/ + delta = (new_raw_count << shift) - (prev_raw_count << shift); + delta >>= shift;
new_raw_count could be smaller than prev_raw_count. The sign bit will be set. The delta>> could be wrong.
I think we can add a period here to prevent it.
+ delta = (period << shift) + (new_raw_count << shift) -
+ (prev_raw_count << shift);
+ delta >>= shift;
......
+ local64_add(delta + period * (count - 1), &event->count);
Thanks,
Kan
Quoted 60 lines
arch/x86/events/intel/ds.c +45
-2+ +/* +* Since the counter increments a negative counter value and +* overflows on the sign switch, giving the interval: +* +* [-period, 0] +* +* the difference between two consequtive reads is: +* +* A) value2 - value1; +* when no overflows have happened in between, +* +* B) (0 - value1) + (value2 - (-period)); +* when one overflow happened in between, +* +* C) (0 - value1) + (n - 1) * (period) + (value2 - (-period)); +* when @n overflows happened in between. +* +* Here A) is the obvious difference, B) is the extention to the +* discrete interval, where the first term is to the top of the +* interval and the second term is from the bottom of the next +* interval and 3) the extention to multiple intervals, where the +* middle term is the whole intervals covered. +* +* An equivalent of C, by reduction, is: +* +* value2 - value1 + n * period +*/ +local64_add (delta + period * count, &event->count); + +perf_event_update_userpage (event); + +return 0 ; +} +static void __intel_pmu_pebs_event (struct perf_event *event,struct pt_regs *iregs,void *base,void *top,int bit,int count) { +struct hw_perf_event *hwc = &event->hw;struct perf_sample_data data;struct pt_regs regs;void *at =get_next_pebs_record_by_bit (base, top, bit); -if (!intel_pmu_save_and_restart (event) && - !(event->hw.flags &PERF_X86_EVENT_AUTO_RELOAD )) +if (hwc->flags &PERF_X86_EVENT_AUTO_RELOAD ) { +/* +* Now, auto-reload is only enabled in fixed period mode. +* The reload value is always hwc->sample_period. +* May need to change it, if auto-reload is enabled in +* freq mode later. +*/ +intel_pmu_save_and_restart_reload (event, count); + }else if (!intel_pmu_save_and_restart (event))return ;while (count >1 ) {
Peter Zijlstra
maintainerRe: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload
Quoted 7 lines · Kan
Quoted 5 lines
With the exception of handling 'empty' buffers, I ended up with the below. Please try again.
There are two small errors. After fixing them, the patch works well.
Well, it still doesn't do A, two read()s without PEBS record in between. So that needs fixing. What 3/5 does, call x86_perf_event_update() after drain_pebs() is actively wrong after this patch.
Quoted 13 lines
arch/x86/events/intel/ds.c get_next_pebs_record_by_bit(void *base, · line 1337
Quoted 7 lines
arch/x86/events/intel/ds.c +7
-0+ +/* +* Careful, not all hw sign-extends above the physical width +* of the counter. +*/ + delta = (new_raw_count << shift) - (prev_raw_count << shift); + delta >>= shift;
new_raw_countcould be smaller thanprev_raw_count. The sign bit will be set. The delta>> could be wrong.I think we can add a period here to prevent it.
Review
Quoted 5 lines
(quoted diff) +4
-0+ delta = (period << shift) + (new_raw_count << shift) - + (prev_raw_count << shift); + delta >>= shift; + local64_add(delta + period * (count - 1), &event->count);
Right it does, but that wrecks case A again, because then we get here with !@count.
Maybe something like:
s64 new, old; new = ((s64)(new_raw_count << shift) >> shift); old = ((s64)(old_raw_count << shift) >> shift); local64_add(new - old + count * period, &event->count);
And then make intel_pmu_drain_pebs_*(), call this function even when !n.
Re: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Kan
Quoted 5 lines
With the exception of handling 'empty' buffers, I ended up with the below. Please try again.
Quoted 6 lines
Quoted 1 line
There are two small errors. After fixing them, the patch works well.
Well, it still doesn't do A, two read()s without PEBS record in between. So that needs fixing. What 3/5 does, call
x86_perf_event_update()afterdrain_pebs()is actively wrong after this patch.
As my understanding, for case A, drain_pebs() will return immediately. It cannot reach the patch. Because there is no PEBS record ready. So the ds->pebs_index should be the same as ds->pebs_buffer_base.
3/5 is to handle case A.
Thanks,
Kan
Quoted 35 lines
Quoted 13 lines
arch/x86/events/intel/ds.c get_next_pebs_record_by_bit(void *base, · line 1337
Quoted 7 lines
arch/x86/events/intel/ds.c +7
-0+ +/* +* Careful, not all hw sign-extends above the physical width +* of the counter. +*/ + delta = (new_raw_count << shift) - (prev_raw_count << shift); + delta >>= shift;
new_raw_countcould be smaller thanprev_raw_count. The sign bit will be set. The delta>> could be wrong.I think we can add a period here to prevent it.
Review
Quoted 5 lines
(quoted diff) +4
-0+ delta = (period << shift) + (new_raw_count << shift) - + (prev_raw_count << shift); + delta >>= shift; + local64_add(delta + period * (count - 1), &event->count);Right it does, but that wrecks case A again, because then we get here with !@count.
Maybe something like:
s64 new, old; new = ((s64)(new_raw_count << shift) >> shift); old = ((s64)(old_raw_count << shift) >> shift); local64_add(new - old + count * period, &event->count);And then make intel_pmu_drain_pebs_*(), call this function even when !n.
Peter Zijlstra
maintainerRe: [PATCH V3 1/5] perf/x86/intel: fix event update for auto-reload
Quoted 17 lines · Kan
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Kan
Quoted 5 lines
With the exception of handling 'empty' buffers, I ended up with the below. Please try again.
Quoted 6 lines
Quoted 1 line
There are two small errors. After fixing them, the patch works well.
Well, it still doesn't do A, two read()s without PEBS record in between. So that needs fixing. What 3/5 does, call
x86_perf_event_update()afterdrain_pebs()is actively wrong after this patch.
Quoted 4 lines
As my understanding, for case A,
drain_pebs()will return immediately. It cannot reach the patch.
Because there is no PEBS record ready. So theds->pebs_indexshould be the same asds->pebs_buffer_base.
Right, so fix that.
Quoted 1 line
3/5 is to handle case A.
3/5 is terminatlly broken, you should not call x86_perf_event_update() on a auto-reload event _ever_ after my patch.
PATCH v3 2/5 perf/x86: introduce read function for x86_pmu
In reply to [email protected]From: Kan Liang <[email protected]>
Large PEBS needs to be specially handled in event count read.
--- arch/x86/events/core.c | 2 ++ arch/x86/events/perf_event.h | 1 + 2 files changed, 3 insertions(+)
arch/x86/events/core.c +2 -0
@@ -1884,6 +1884,8 @@ early_initcall(init_hw_perf_events);static inline void x86_pmu_read (struct perf_event *event) { +if (x86_pmu.read) +return x86_pmu.read (event);x86_perf_event_update (event); }
arch/x86/events/perf_event.h +1 -0
@@ -519,6 +519,7 @@ struct x86_pmu {void (*disable)(struct perf_event *);void (*add)(struct perf_event *);void (*del)(struct perf_event *); +void (*read)(struct perf_event *event);int (*hw_config)(struct perf_event *event);int (*schedule_events)(struct cpu_hw_events *cpuc,int n,int *assign);unsigned eventsel;
Signature · 1 line
2.7.4
PATCH v3 3/5 perf/x86/intel/ds: introduce read function for large pebs
In reply to [email protected]From: Kan Liang <[email protected]>
When the PEBS interrupt threshold is larger than one, there is no way to get exact auto-reload times and value, which needed for event update unless flush the PEBS buffer.
Introduce intel_pmu_large_pebs_read() to drain the PEBS buffer in event read when large PEBS is enabled.
To prevent the race, the drain_pebs() only be called when the PMU is disabled.
Unconditionally call x86_perf_event_update() for large pebs.
- It is easily to call pmu::read() twice in a short period. There could be no samples in the PEBS buffer. x86_perf_event_update() is needed to update the count.
- There is no harmful to call x86_perf_event_update() for other cases.
- It's safe. Don't need to worry about the auto-reload. Because the PMU is disabled.
--- arch/x86/events/intel/ds.c | 16 ++++++++++++++++ arch/x86/events/perf_event.h | 2 ++ 2 files changed, 18 insertions(+)
arch/x86/events/intel/ds.c +16 -0
@@ -1303,6 +1303,22 @@ get_next_pebs_record_by_bit(void *base, void *top, int bit)return NULL ; } +int intel_pmu_large_pebs_read (struct perf_event *event) +{ +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +/* Check if the event has large pebs */ +if (!pebs_needs_sched_cb (cpuc)) +return 0 ; + +perf_pmu_disable (event->pmu); +intel_pmu_drain_pebs_buffer (); +x86_perf_event_update (event); +perf_pmu_enable (event->pmu); + +return 1 ; +} +/* * Specific intel_pmu_save_and_restart() for auto-reload. * It only be called from drain_pebs().
arch/x86/events/perf_event.h +2 -0
@@ -923,6 +923,8 @@ void intel_pmu_pebs_disable_all(void);void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in); +int intel_pmu_large_pebs_read (struct perf_event *event); +void intel_ds_init (void );void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in);
Signature · 1 line
2.7.4
PATCH v3 4/5 perf/x86/intel: fix pmu read for large PEBS
In reply to [email protected]From: Kan Liang <[email protected]>
Large PEBS needs to be specially handled in event count read.
It is only available for intel_pmu.
Only need to specially handle the large PEBS. For the threshold is one, even auto-reload is enabled, it doesn't need to be specially handled. Because,
- auto-reload is only effect when event overflow. Once there is an overflow, the NMI handler will automatically drain_pebs().
- For other cases, x86_perf_event_update() is good enough.
handling (large PEBS interrupt threshold)")
--- arch/x86/events/intel/core.c | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/x86/events/intel/core.c +9 -0
@@ -2060,6 +2060,14 @@ static void intel_pmu_del_event(struct perf_event *event)intel_pmu_pebs_del (event); } +static void intel_pmu_read_event (struct perf_event *event) +{ +if (intel_pmu_large_pebs_read (event)) +return ; + +x86_perf_event_update (event); +} +static void intel_pmu_enable_fixed (struct hw_perf_event *hwc) {int idx = hwc->idx -INTEL_PMC_IDX_FIXED ;
@@ -3495,6 +3503,7 @@ static __initconst const struct x86_pmu intel_pmu = { .disable = intel_pmu_disable_event, .add = intel_pmu_add_event, .del = intel_pmu_del_event, + .read = intel_pmu_read_event, .hw_config = intel_pmu_hw_config, .schedule_events = x86_schedule_events, .eventsel = MSR_ARCH_PERFMON_EVENTSEL0,
Signature · 1 line
2.7.4
PATCH v3 5/5 perf/x86: fix: disable userspace RDPMC usage for large PEBS
In reply to [email protected]From: Kan Liang <[email protected]>
The userspace RDPMC usage never works for large PEBS since the large PEBS is introduced by
commit b8241d20699e ("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")
When the PEBS interrupt threshold is larger than one, there is no way to get exact auto-reload times and value for userspace RDPMC.
Disable the userspace RDPMC usage when large PEBS is enabled.
handling (large PEBS interrupt threshold)")
--- arch/x86/events/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/events/core.c +2 -1
@@ -2120,7 +2120,8 @@ static int x86_pmu_event_init(struct perf_event *event) event->destroy (event); } -if (READ_ONCE (x86_pmu.attr_rdpmc)) +if (READ_ONCE (x86_pmu.attr_rdpmc) && + !(event->hw.flags &PERF_X86_EVENT_FREERUNNING )) event->hw.flags |=PERF_X86_EVENT_RDPMC_ALLOWED ;return err;
Signature · 1 line
2.7.4
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0
Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()
It is not yet clear to me why PERF_SAMPLE_PERIOD is not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.
I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0
But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to setPERF_SAMPLE_PERIOD in the event.
If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0
I get
perf_event_attr:
type 4
size 112
config 0x10d1
{ sample_period, sample_freq } 199936
sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0
I get
perf_event_attr:
type 4
size 112
config 0x10d1
{ sample_period, sample_freq } 199936
sample_type IP|TID|CPU|PERIODPerf should check if all events have a period=, then it should not pass PERF_SAMPLE_PERIOD, even
more so when only one event is defined.
Also it does not seem to honor --no-period.
All of this makes it hard to use large PEBS in general.
So I think your series should also address this part.
Quoted 117 lines
- Introduce a special purpose
intel_pmu_save_and_restart()just forAUTO_RELOAD.
- New patch to disable userspace RDPMC usage if large PEBS is enabled.------
There is a bug when mmap read
event->countwith large PEBS enabled. Here is an example.
#./read_count
0x71f0
0x122c0
0x1000000001c54
0x100000001257d
0x200000000bdc5The bug is caused by two issues.
- Inx86_perf_event_update, the calculation ofevent->countdoes not take the auto-reload values into account.
- Inx86_pmu_read, it doesn't count the undrained values in large PEBS buffers.The first issue was introduced with the auto-reload mechanism enabled since commit
851559e35fd5("perf/x86/intel: Use the PEBS auto reload mechanism when possible")Patch 1 fixed the issue in
x86_perf_event_update.The second issue was introduced since commit
b8241d20699e("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")Patch 2-4 fixed the issue in
x86_pmu_read.Besides the two issues, the userspace RDPMC usage is broken for large PEBS as well.
The RDPMC issue was also introduced since commitb8241d20699e("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")Patch 5 fixed the RDPMC issue.
The source code of
read_countis as below.struct cpu { int fd; struct perf_event_mmap_page *buf; };int
perf_open(struct cpu *ctx, int cpu){ struct perf_event_attr attr = { .type = PERF_TYPE_HARDWARE, .size = sizeof(struct perf_event_attr), .sample_period = 100000, .config = 0, .sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID | PERF_SAMPLE_TIME | PERF_SAMPLE_CPU, .precise_ip = 3, .mmap = 1, .comm = 1, .task = 1, .mmap2 = 1, .sample_id_all = 1, .comm_exec = 1, }; ctx->buf = NULL; ctx->fd = syscall(__NR_perf_event_open, &attr, -1, cpu, -1, 0); if (ctx->fd < 0) { perror("perf_event_open"); return -1; } return 0; }void
perf_close(struct cpu *ctx){ close(ctx->fd); if (ctx->buf) munmap(ctx->buf, pagesize); }int
main(int ac, char **av){ struct cpu ctx; u64 count; perf_open(&ctx, 0); while (1) { sleep(5); if (read(ctx.fd, &count, 8) != 8) { perror("counter read"); break; } printf("0x%llx\n", count); } perf_close(&ctx); }Kan Liang (5):
perf/x86/intel: fix event update for auto-reload perf/x86: introduce read function forx86_pmuperf/x86/intel/ds: introduce read function for large pebs perf/x86/intel: introduce read function forintel_pmuperf/x86: fix: disable userspace RDPMC usage for large PEBSarch/x86/events/core.c | 5 ++- arch/x86/events/intel/core.c | 9 +++++ arch/x86/events/intel/ds.c | 85 ++++++++++++++++++++++++++++++++++++++++++-- arch/x86/events/perf_event.h | 3 ++ 4 files changed, 99 insertions(+), 3 deletions(-)--
2.7.4
Quoted 65 lines · Stephane Eranian
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIOD
Quoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set the sample_type based on that
attached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2 -1
@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), -OPT_BOOLEAN ('P' ,"period" , &record.opts.period,"Record the sample period" ), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ),OPT_BOOLEAN ('n' ,"no-samples" , &record.opts.no_samples,"don't sample" ), OPT_BOOLEAN_SET('N', "no-buildid-cache", &record.no_buildid_cache,
tools/perf/perf.h +1 -0
@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;
tools/perf/util/evsel.c +10 -3
@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel,if (!(term->weak && opts->user_interval !=ULLONG_MAX )) { attr->sample_period = term->val.period; attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq; attr->freq =1 ; +perf_evsel__set_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_TIME :
@@ -969,9 +971,6 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,if (target__has_cpu (&opts->target) || opts->sample_cpu)perf_evsel__set_sample_bit (evsel,CPU ); -if (opts->period) -perf_evsel__set_sample_bit (evsel,PERIOD ); -/* * When the user explicitly disabled time don't force it here. */
@@ -1073,6 +1072,14 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,apply_config_terms (evsel, opts, track); evsel->ignore_missing_thread = opts->ignore_missing_thread; + +/* The --period option takes the precedence. */ +if (opts->period_set) { +if (opts->period) +perf_evsel__set_sample_bit (evsel,PERIOD ); +else +perf_evsel__reset_sample_bit (evsel,PERIOD ); + } }static int perf_evsel__alloc_fd (struct perf_evsel *evsel,int ncpus,int nthreads)
Quoted 115 lines · Jiri Olsa
Quoted 65 lines · Stephane Eranian
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIODQuoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set thesample_typebased on thatattached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2
-1@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), -OPT_BOOLEAN ('P' ,"period" , &record.opts.period,"Record the sample period" ), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ),OPT_BOOLEAN ('n' ,"no-samples" , &record.opts.no_samples,"don't sample" ), OPT_BOOLEAN_SET('N', "no-buildid-cache", &record.no_buildid_cache,tools/perf/perf.h +1
-0@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;tools/perf/util/evsel.c +0
-0@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel,if (!(term->weak && opts->user_interval !=ULLONG_MAX )) { attr->sample_period = term->val.period;
tools/perf/util/evsel.c static void apply_config_terms(struct perf_evsel *evsel, · line 756
Quoted 10 lines
tools/perf/util/evsel.c +2
-0attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq; attr->freq =1 ; +perf_evsel__set_sample_bit (evsel,PERIOD ); }
If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set. That could be a problem for large PEBS. The PEBS buffer is shared among events. It doesn't support freq mode yet.
Thanks,
Kan
Quoted 28 lines
tools/perf/util/evsel.c +8
-3break ;case PERF_EVSEL__CONFIG_TERM_TIME :@@ -969,9 +971,6 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,if (target__has_cpu (&opts->target) || opts->sample_cpu)perf_evsel__set_sample_bit (evsel,CPU ); -if (opts->period) -perf_evsel__set_sample_bit (evsel,PERIOD ); -/* * When the user explicitly disabled time don't force it here. */ @@ -1073,6 +1072,14 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,apply_config_terms (evsel, opts, track); evsel->ignore_missing_thread = opts->ignore_missing_thread; + +/* The --period option takes the precedence. */ +if (opts->period_set) { +if (opts->period) +perf_evsel__set_sample_bit (evsel,PERIOD ); +else +perf_evsel__reset_sample_bit (evsel,PERIOD ); + } }static int perf_evsel__alloc_fd (struct perf_evsel *evsel,int ncpus,int nthreads)
Quoted 123 lines · Kan
Quoted 120 lines · Jiri Olsa
Quoted 65 lines · Stephane Eranian
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIODQuoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set thesample_typebased on thatattached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2
-1@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), -OPT_BOOLEAN ('P' ,"period" , &record.opts.period,"Record the sample period" ), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ),OPT_BOOLEAN ('n' ,"no-samples" , &record.opts.no_samples,"don't sample" ), OPT_BOOLEAN_SET('N', "no-buildid-cache", &record.no_buildid_cache,tools/perf/perf.h +1
-0@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;tools/perf/util/evsel.c +1
-0@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel,if (!(term->weak && opts->user_interval !=ULLONG_MAX )) { attr->sample_period = term->val.period; attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :
Quoted 8 lines
tools/perf/util/evsel.c static void apply_config_terms(struct perf_evsel *evsel, · line 756
Quoted 5 lines
tools/perf/util/evsel.c +1
-0if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq; attr->freq =1 ; +perf_evsel__set_sample_bit (evsel,PERIOD ); }If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set.
it also sets the attr->freq, so it's not in fixed mode
jirka
Quoted 133 lines · Jiri Olsa
Quoted 123 lines · Kan
Quoted 120 lines · Jiri Olsa
Quoted 65 lines · Stephane Eranian
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIODQuoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set thesample_typebased on thatattached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2
-1@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), -OPT_BOOLEAN ('P' ,"period" , &record.opts.period,"Record the sample period" ), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ),OPT_BOOLEAN ('n' ,"no-samples" , &record.opts.no_samples,"don't sample" ), OPT_BOOLEAN_SET('N', "no-buildid-cache", &record.no_buildid_cache,tools/perf/perf.h +1
-0@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;tools/perf/util/evsel.c +1
-0@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel,if (!(term->weak && opts->user_interval !=ULLONG_MAX )) { attr->sample_period = term->val.period; attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :Quoted 8 lines
tools/perf/util/evsel.c static void apply_config_terms(struct perf_evsel *evsel, · line 756
Quoted 5 lines
tools/perf/util/evsel.c +1
-0if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq; attr->freq =1 ; +perf_evsel__set_sample_bit (evsel,PERIOD ); }If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set.
Quoted 2 lines
it also sets the
attr->freq, so it's not in fixed mode
I mean the TERM_PERIOD. It probably enable the large PEBS.
Quoted 2 lines
Quoted 2 lines
tools/perf/util/evsel.c static void apply_config_terms(struct perf_evsel *evsel, · line 748
Quoted 2 lines
tools/perf/util/evsel.c +1
-0attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD );
The events in fixed mode could enable large PEBS. Events in freq mode should not enable large PEBS. I think that could be a problem if some events try to enable large PEBS, while others not.
Thanks,
Kan
Quoted 156 lines · Kan
Quoted 150 lines · Jiri Olsa
Quoted 138 lines · Kan
Quoted 134 lines · Jiri Olsa
Quoted 68 lines · Stephane Eranian
Hi,
Quoted 17 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIODQuoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set thesample_typebased on thatattached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2
-1@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), - OPT_BOOLEAN('P', "period", &record.opts.period, "Record the sample period"), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ), OPT_BOOLEAN('n', "no-samples", &record.opts.no_samples,"don't sample"), OPT_BOOLEAN_SET('N', "no-buildid-cache",&record.no_buildid_cache,
tools/perf/perf.h +1
-0@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;tools/perf/util/evsel.c +1
-0@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel, if (!(term->weak && opts->user_interval != ULLONG_MAX)) { attr->sample_period = term->val.period; attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq;Quoted 8 lines
Quoted 4 lines
attr->freq = 1; + perf_evsel__set_sample_bit(evsel, PERIOD); }If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set.
Quoted 2 lines
it also sets the
attr->freq, so it's not in fixed mode
Quoted 9 lines
I mean the
TERM_PERIOD. It probably enable the large PEBS.Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
attr->freq = 0;+
perf_evsel__reset_sample_bit(evsel, PERIOD);The events in fixed mode could enable large PEBS. Events in freq mode should not enable large PEBS.
I think that could be a problem if some events try to enable large PEBS, while others not.
You only enable large PEBS if 100% of the events use fixed periods, either via -c period
or because they all use individual period=p. The --no-period could also be used to remove
the period for measurements where the period is not needed.
Quoted 4 lines
Thanks,
Kan
Quoted 166 lines · Stephane Eranian
Quoted 156 lines · Kan
Quoted 150 lines · Jiri Olsa
Quoted 138 lines · Kan
Quoted 134 lines · Jiri Olsa
Quoted 68 lines · Stephane Eranian
Hi,
Quoted 17 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore callintel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIODQuoted 5 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.Also it does not seem to honor --no-period.
yep, there's a bug in period=x term handling
we did not re/set thesample_typebased on thatattached patch fixes that for me, also takes into account the --no/-period options
jirka
---
tools/perf/builtin-record.c +2
-1@@ -1566,7 +1566,8 @@ static struct option __record_options[] = {OPT_BOOLEAN_SET ('T' ,"timestamp" , &record.opts.sample_time, &record.opts.sample_time_set,"Record the sample timestamps" ), - OPT_BOOLEAN('P', "period", &record.opts.period, "Record the sample period"), +OPT_BOOLEAN_SET ('P' ,"period" , &record.opts.period, &record.opts.period_set, +"Record the sample period" ), OPT_BOOLEAN('n', "no-samples", &record.opts.no_samples,"don't sample"), OPT_BOOLEAN_SET('N', "no-buildid-cache",&record.no_buildid_cache,
tools/perf/perf.h +1
-0@@ -50,6 +50,7 @@ struct record_opts {bool sample_time_set;bool sample_cpu;bool period; +bool period_set;bool running_time;bool full_auxtrace;bool auxtrace_snapshot_mode;tools/perf/util/evsel.c +1
-0@@ -745,12 +745,14 @@ static void apply_config_terms(struct perf_evsel *evsel, if (!(term->weak && opts->user_interval != ULLONG_MAX)) { attr->sample_period = term->val.period; attr->freq =0 ; +perf_evsel__reset_sample_bit (evsel,PERIOD ); }break ;case PERF_EVSEL__CONFIG_TERM_FREQ :if (!(term->weak && opts->user_freq !=UINT_MAX )) { attr->sample_freq = term->val.freq;Quoted 8 lines
Quoted 4 lines
attr->freq = 1; + perf_evsel__set_sample_bit(evsel, PERIOD); }If we do so, some events could be in fixed mode without PERIOD set. Other events could be in freq mode with PERIOD set.
Quoted 2 lines
it also sets the
attr->freq, so it's not in fixed modeQuoted 9 lines
I mean the
TERM_PERIOD. It probably enable the large PEBS.Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
attr->freq = 0;+
perf_evsel__reset_sample_bit(evsel, PERIOD);The events in fixed mode could enable large PEBS. Events in freq mode should not enable large PEBS.
I think that could be a problem if some events try to enable large PEBS, while others not.
Quoted 5 lines
You only enable large PEBS if 100% of the events use fixed periods, either via -c period
or because they all use individual period=p. The --no-period could also be used to remove
the period for measurements where the period is not needed.
Oh, right, the kernel has already guaranteed that.
if (cpuc->n_pebs == cpuc->n_large_pebs) {
threshold = ds->pebs_absolute_maximum -
x86_pmu.max_pebs_events * x86_pmu.pebs_record_size;
} else {Sorry for the noise.
jirka's patch looks good to me.
Thanks,
Kan
On Tue, Jan 30, 2018 at 11:48:18AM -0500, Liang, Kan wrote:
SNIP
Quoted 14 lines
Quoted 11 lines
Quoted 6 lines
The events in fixed mode could enable large PEBS. Events in freq mode should not enable large PEBS.
I think that could be a problem if some events try to enable large PEBS, while others not.You only enable large PEBS if 100% of the events use fixed periods, either via -c period
or because they all use individual period=p. The --no-period could also be used to remove
the period for measurements where the period is not needed.Oh, right, the kernel has already guaranteed that.
Quoted 8 lines
if (cpuc->n_pebs == cpuc->n_large_pebs) { threshold = ds->pebs_absolute_maximum - x86_pmu.max_pebs_events * x86_pmu.pebs_record_size; } else {Sorry for the noise.
jirka's patch looks good to me.
cool, I'll post it later this week
thanks,
jirka
Quoted 24 lines · Jiri Olsa
On Tue, Jan 30, 2018 at 11:48:18AM -0500, Liang, Kan wrote:
SNIP
Quoted 19 lines
Quoted 11 lines
Quoted 6 lines
The events in fixed mode could enable large PEBS. Events in freq mode should not enable large PEBS.
I think that could be a problem if some events try to enable large PEBS, while others not.You only enable large PEBS if 100% of the events use fixed periods, either via -c period
or because they all use individual period=p. The --no-period could also be used to remove
the period for measurements where the period is not needed.Oh, right, the kernel has already guaranteed that.
if (cpuc->n_pebs == cpuc->n_large_pebs) { threshold = ds->pebs_absolute_maximum - x86_pmu.max_pebs_events * x86_pmu.pebs_record_size; } else {
Quoted 6 lines
Quoted 3 lines
Sorry for the noise.
jirka's patch looks good to me.
cool, I'll post it later this week
Still, the part I am missing here, is why asking for PERF_SAMPLE_PERIOD voids large PEBS.
Quoted 2 lines
Still, the part I am missing here, is why asking for
PERF_SAMPLE_PERIODvoids large PEBS.
I think it was disabled together with frequency mode (which we could support too, but it's a bit more work)
But yes PERIOD could probably be allowed.
-Andi
Quoted 7 lines · Andi Kleen
Quoted 2 lines
Still, the part I am missing here, is why asking for
PERF_SAMPLE_PERIODvoids large PEBS.I think it was disabled together with frequency mode (which we could support too, but it's a bit more work)
But yes PERIOD could probably be allowed.
looks like it's just a matter of adding PERF_SAMPLE_PERIOD into PEBS_FREERUNNING_FLAGS, we already populate period in setup_pebs_sample_data
jirka
---
arch/x86/events/perf_event.h +2 -1
@@ -93,7 +93,8 @@ struct amd_nb { PERF_SAMPLE_ID | PERF_SAMPLE_CPU | PERF_SAMPLE_STREAM_ID | \ PERF_SAMPLE_DATA_SRC | PERF_SAMPLE_IDENTIFIER | \ PERF_SAMPLE_TRANSACTION | PERF_SAMPLE_PHYS_ADDR | \ - PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER) + PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER | \ + PERF_SAMPLE_PERIOD)#define PEBS_REGS \ (PERF_REG_X86_AX | \
Quoted 22 lines · Jiri Olsa
Quoted 7 lines · Andi Kleen
Quoted 2 lines
Still, the part I am missing here, is why asking for
PERF_SAMPLE_PERIODvoids large PEBS.I think it was disabled together with frequency mode (which we could support too, but it's a bit more work)
But yes PERIOD could probably be allowed.
looks like it's just a matter of adding
PERF_SAMPLE_PERIODintoPEBS_FREERUNNING_FLAGS, we already populate period insetup_pebs_sample_datajirka
---
arch/x86/events/perf_event.h +0
-0
arch/x86/events/perf_event.h struct amd_nb { · line 97
Quoted 6 lines
arch/x86/events/perf_event.h +2
-1@@ -93,7 +93,8 @@ struct amd_nb {PERF_SAMPLE_ID |PERF_SAMPLE_CPU |PERF_SAMPLE_STREAM_ID | \PERF_SAMPLE_DATA_SRC |PERF_SAMPLE_IDENTIFIER | \PERF_SAMPLE_TRANSACTION |PERF_SAMPLE_PHYS_ADDR | \ - PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER) +PERF_SAMPLE_REGS_INTR |PERF_SAMPLE_REGS_USER | \ + PERF_SAMPLE_PERIOD)
seems to work, getting large PEBS event for following command line:
perf record -e cycles:P -c 100 --no-timestamp -C 0 --period
jirka
Quoted 32 lines · Jiri Olsa
Quoted 22 lines · Jiri Olsa
Quoted 7 lines · Andi Kleen
Quoted 2 lines
Still, the part I am missing here, is why asking for
PERF_SAMPLE_PERIODvoids large PEBS.I think it was disabled together with frequency mode (which we could support too, but it's a bit more work)
But yes PERIOD could probably be allowed.
looks like it's just a matter of adding
PERF_SAMPLE_PERIODintoPEBS_FREERUNNING_FLAGS, we already populate period insetup_pebs_sample_datajirka
---
arch/x86/events/perf_event.h +0
-0arch/x86/events/perf_event.h struct amd_nb { · line 97
Quoted 6 lines
arch/x86/events/perf_event.h +2
-1@@ -93,7 +93,8 @@ struct amd_nb {PERF_SAMPLE_ID |PERF_SAMPLE_CPU |PERF_SAMPLE_STREAM_ID | \PERF_SAMPLE_DATA_SRC |PERF_SAMPLE_IDENTIFIER | \PERF_SAMPLE_TRANSACTION |PERF_SAMPLE_PHYS_ADDR | \ - PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER) +PERF_SAMPLE_REGS_INTR |PERF_SAMPLE_REGS_USER | \ + PERF_SAMPLE_PERIOD)seems to work, getting large PEBS event for following command line:
Quoted 3 lines
perf record -e cycles:P -c 100 --no-timestamp -C 0 --period
Yes, I tried the patch. The large PEBS can be enabled with the PERIOD flag. Everything looks good.
Jirka, could you please post the patch as well?
Thanks,
Kan
Quoted 35 lines · Kan
Quoted 32 lines · Jiri Olsa
Quoted 22 lines · Jiri Olsa
Quoted 7 lines · Andi Kleen
Quoted 2 lines
Still, the part I am missing here, is why asking for
PERF_SAMPLE_PERIODvoids large PEBS.I think it was disabled together with frequency mode (which we could support too, but it's a bit more work)
But yes PERIOD could probably be allowed.
looks like it's just a matter of adding
PERF_SAMPLE_PERIODintoPEBS_FREERUNNING_FLAGS, we already populate period insetup_pebs_sample_datajirka
---
arch/x86/events/perf_event.h +0
-0arch/x86/events/perf_event.h struct amd_nb { · line 97
Quoted 6 lines
arch/x86/events/perf_event.h +2
-1@@ -93,7 +93,8 @@ struct amd_nb {PERF_SAMPLE_ID |PERF_SAMPLE_CPU |PERF_SAMPLE_STREAM_ID | \PERF_SAMPLE_DATA_SRC |PERF_SAMPLE_IDENTIFIER | \PERF_SAMPLE_TRANSACTION |PERF_SAMPLE_PHYS_ADDR | \ - PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER) +PERF_SAMPLE_REGS_INTR |PERF_SAMPLE_REGS_USER | \ + PERF_SAMPLE_PERIOD)seems to work, getting large PEBS event for following command line:
Quoted 8 lines
Quoted 3 lines
perf record -e cycles:P -c 100 --no-timestamp -C 0 --periodYes, I tried the patch. The large PEBS can be enabled with the PERIOD flag. Everything looks good.
Jirka, could you please post the patch as well?
yes, will post that one as well
jirka
Quoted 19 lines · Stephane Eranian
Hi,
Quoted 16 lines · On Mon, Jan 29, 2018 at 8:29 AM, <[email protected]> wrote:
From: Kan Liang <[email protected]>
------
Changes since V2:
- Refined the changelog
- Introduced specific read function for large PEBS. The previous generic PEBS read function is confusing. Disabled PMU inpmu::read()path for large PEBS. Handled the corner case whenreload_times== 0.
- Modified the parameter ofintel_pmu_save_and_restart_reload()Discardedlocal64_cmpxchg
- Added fixes tag
- Added WARN to handlereload_times== 0 ||reload_val== 0
Quoted 7 lines
Quoted 3 lines
Changes since V1:
- CheckPERF_X86_EVENT_AUTO_RELOADbefore call
intel_pmu_save_and_restore()It is not yet clear to me why
PERF_SAMPLE_PERIODis not allowed with large PEBS. Large PEBS requires fixed period. So the kernel could make up the period from the event and store it in the sampling buffer.
The PERF_SAMPLE_PERIOD will be implicitly set in freq mode. By now, large PEBS doesn't support freq mode yet.
Quoted 8 lines
I tried using large PEBS recently, and despite trying different option combination of perf record, I was not able to get it to work.
$ perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0But I was able to make this work with a much older kernel.
Is there any error message?
Quoted 31 lines
Another annoyance I ran into is with perf record requiring -c period in order not to set
PERF_SAMPLE_PERIODin the event.If I do:
perf record -c 1 -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPUBut if I do:
perf record -e cpu/event=0xd1,umask=0x10,period=19936/pp --no-timestamp --no-period -a -C 0I get
perf_event_attr:type 4 size 112 config 0x10d1 { sample_period, sample_freq } 199936 sample_type IP|TID|CPU|PERIOD
Quoted 3 lines
Perf should check if all events have a period=, then it should not pass
PERF_SAMPLE_PERIOD, even
more so when only one event is defined.
As my understanding, the "period=" is per-event term. It should not change the global setting. I think it's better still use "--no-period" to control the PERIOD bit.
Quoted 3 lines
Also it does not seem to honor --no-period.
Right, perf tool doesn't clear the PERIOD for --no-period.
if (opts->period) perf_evsel__set_sample_bit(evsel, PERIOD);
I will submit a patch to fix it.
Quoted 3 lines
All of this makes it hard to use large PEBS in general.
So I think your series should also address this part.
The issue looks like perf tool problem. I will take a look at it, but probably address it in a separate patch set.
Thanks,
Kan
Quoted 118 lines
Quoted 118 lines
- Introduce a special purpose
intel_pmu_save_and_restart()just forAUTO_RELOAD.
- New patch to disable userspace RDPMC usage if large PEBS is enabled.------
There is a bug when mmap read
event->countwith large PEBS enabled. Here is an example.
#./read_count
0x71f0
0x122c0
0x1000000001c54
0x100000001257d
0x200000000bdc5The bug is caused by two issues.
- Inx86_perf_event_update, the calculation ofevent->countdoes not take the auto-reload values into account.
- Inx86_pmu_read, it doesn't count the undrained values in large PEBS buffers.The first issue was introduced with the auto-reload mechanism enabled since commit
851559e35fd5("perf/x86/intel: Use the PEBS auto reload mechanism when possible")Patch 1 fixed the issue in
x86_perf_event_update.The second issue was introduced since commit
b8241d20699e("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")Patch 2-4 fixed the issue in
x86_pmu_read.Besides the two issues, the userspace RDPMC usage is broken for large PEBS as well.
The RDPMC issue was also introduced since commitb8241d20699e("perf/x86/intel: Implement batched PEBS interrupt handling (large PEBS interrupt threshold)")Patch 5 fixed the RDPMC issue.
The source code of
read_countis as below.struct cpu { int fd; struct perf_event_mmap_page *buf; };int
perf_open(struct cpu *ctx, int cpu){ struct perf_event_attr attr = { .type = PERF_TYPE_HARDWARE, .size = sizeof(struct perf_event_attr), .sample_period = 100000, .config = 0, .sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID | PERF_SAMPLE_TIME | PERF_SAMPLE_CPU, .precise_ip = 3, .mmap = 1, .comm = 1, .task = 1, .mmap2 = 1, .sample_id_all = 1, .comm_exec = 1, }; ctx->buf = NULL; ctx->fd = syscall(__NR_perf_event_open, &attr, -1, cpu, -1, 0); if (ctx->fd < 0) { perror("perf_event_open"); return -1; } return 0; }void
perf_close(struct cpu *ctx){ close(ctx->fd); if (ctx->buf) munmap(ctx->buf, pagesize); }int
main(int ac, char **av){ struct cpu ctx; u64 count; perf_open(&ctx, 0); while (1) { sleep(5); if (read(ctx.fd, &count, 8) != 8) { perror("counter read"); break; } printf("0x%llx\n", count); } perf_close(&ctx); }Kan Liang (5):
perf/x86/intel: fix event update for auto-reload perf/x86: introduce read function forx86_pmuperf/x86/intel/ds: introduce read function for large pebs perf/x86/intel: introduce read function forintel_pmuperf/x86: fix: disable userspace RDPMC usage for large PEBSarch/x86/events/core.c | 5 ++- arch/x86/events/intel/core.c | 9 +++++ arch/x86/events/intel/ds.c | 85 ++++++++++++++++++++++++++++++++++++++++++-- arch/x86/events/perf_event.h | 3 ++ 4 files changed, 99 insertions(+), 3 deletions(-)--
2.7.4