From: Kan Liang <[email protected]>
When a group that has TopDown members is failed to be scheduled, any
later TopDown groups will not return valid values.
Here is an example.
A background perf that occupies all the GP counters and the fixed
counter 1.
$perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
cycles,cycles}:D" -a
A user monitors a TopDown group. It works well, because the fixed
counter 3 and the PERF_METRICS are available.
$perf stat -x, --topdown -- ./workload
retiring,bad speculation,frontend bound,backend bound,
18.0,16.1,40.4,25.5,
Then the user tries to monitor a group that has TopDown members.
Because of the cycles event, the group is failed to be scheduled.
$perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
topdown-fe-bound,topdown-bad-spec,cycles}'
-- ./workload
<not counted>,,slots,0,0.00,,
<not counted>,,topdown-retiring,0,0.00,,
<not counted>,,topdown-be-bound,0,0.00,,
<not counted>,,topdown-fe-bound,0,0.00,,
<not counted>,,topdown-bad-spec,0,0.00,,
<not counted>,,cycles,0,0.00,,
The user tries to monitor a TopDown group again. It doesn't work anymore.
$perf stat -x, --topdown -- ./workload
,,,,,
In a txn, cancel_txn() is to truncate the event_list for a canceled
group and update the number of events added in this transaction.
However, the number of TopDown events added in this transaction is not
updated. The kernel will probably fail to add new Topdown events.
Check if the canceled group has Topdown events. If so, subtract the
TopDown events from n_metric accordingly.
Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
Reported-by: Andi Kleen <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
---
arch/x86/events/core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 0f3d01562ded..4cb3ccbe2d62 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2017,6 +2017,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
{
unsigned int txn_flags;
struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+ int i;
WARN_ON_ONCE(!cpuc->txn_flags); /* no txn in flight */
@@ -2031,6 +2032,15 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
*/
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
+
+ /* Subtract Topdown events in the canceled group from n_metric */
+ if (x86_pmu.intel_cap.perf_metrics && cpuc->n_metric) {
+ for (i = 0; i < cpuc->n_txn; i++) {
+ if (is_metric_event(cpuc->event_list[i + cpuc->n_events]))
+ __this_cpu_dec(cpu_hw_events.n_metric);
+ }
+ WARN_ON_ONCE(__this_cpu_read(cpu_hw_events.n_metric) < 0);
+ }
perf_pmu_enable(pmu);
}
--
2.17.1
On Wed, Sep 30, 2020 at 07:29:35AM -0700, [email protected] wrote:
> From: Kan Liang <[email protected]>
>
> When a group that has TopDown members is failed to be scheduled, any
> later TopDown groups will not return valid values.
>
> Here is an example.
>
> A background perf that occupies all the GP counters and the fixed
> counter 1.
> $perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
> cycles,cycles}:D" -a
>
> A user monitors a TopDown group. It works well, because the fixed
> counter 3 and the PERF_METRICS are available.
> $perf stat -x, --topdown -- ./workload
> retiring,bad speculation,frontend bound,backend bound,
> 18.0,16.1,40.4,25.5,
>
> Then the user tries to monitor a group that has TopDown members.
> Because of the cycles event, the group is failed to be scheduled.
> $perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
> topdown-fe-bound,topdown-bad-spec,cycles}'
> -- ./workload
> <not counted>,,slots,0,0.00,,
> <not counted>,,topdown-retiring,0,0.00,,
> <not counted>,,topdown-be-bound,0,0.00,,
> <not counted>,,topdown-fe-bound,0,0.00,,
> <not counted>,,topdown-bad-spec,0,0.00,,
> <not counted>,,cycles,0,0.00,,
>
> The user tries to monitor a TopDown group again. It doesn't work anymore.
> $perf stat -x, --topdown -- ./workload
>
> ,,,,,
>
> In a txn, cancel_txn() is to truncate the event_list for a canceled
> group and update the number of events added in this transaction.
> However, the number of TopDown events added in this transaction is not
> updated. The kernel will probably fail to add new Topdown events.
>
> Check if the canceled group has Topdown events. If so, subtract the
> TopDown events from n_metric accordingly.
>
> Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
> Reported-by: Andi Kleen <[email protected]>
> Reviewed-by: Andi Kleen <[email protected]>
> Signed-off-by: Kan Liang <[email protected]>
> ---
> arch/x86/events/core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 0f3d01562ded..4cb3ccbe2d62 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2017,6 +2017,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
> {
> unsigned int txn_flags;
> struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> + int i;
>
> WARN_ON_ONCE(!cpuc->txn_flags); /* no txn in flight */
>
> @@ -2031,6 +2032,15 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
> */
> __this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
> __this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
> +
> + /* Subtract Topdown events in the canceled group from n_metric */
> + if (x86_pmu.intel_cap.perf_metrics && cpuc->n_metric) {
> + for (i = 0; i < cpuc->n_txn; i++) {
> + if (is_metric_event(cpuc->event_list[i + cpuc->n_events]))
> + __this_cpu_dec(cpu_hw_events.n_metric);
> + }
> + WARN_ON_ONCE(__this_cpu_read(cpu_hw_events.n_metric) < 0);
> + }
> perf_pmu_enable(pmu);
> }
Urgh, I'd much rather we add n_txn_metric. But also, while looking at
this, don't we have the same problem with n_pair ?
Something like this perhaps...
---
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 757e49755e7c..9b7792c0b6fb 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1066,6 +1066,7 @@ static int add_nr_metric_event(struct cpu_hw_events *cpuc,
if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
return -EINVAL;
cpuc->n_metric++;
+ cpuc->n_txn_metric++;
}
return 0;
@@ -1089,8 +1090,10 @@ static int collect_event(struct cpu_hw_events *cpuc, struct perf_event *event,
return -EINVAL;
cpuc->event_list[n] = event;
- if (is_counter_pair(&event->hw))
+ if (is_counter_pair(&event->hw)) {
cpuc->n_pair++;
+ cpuc->n_txn_pair++;
+ }
return 0;
}
@@ -2062,6 +2065,8 @@ static void x86_pmu_start_txn(struct pmu *pmu, unsigned int txn_flags)
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_metric, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_pair, 0);
}
/*
@@ -2087,6 +2092,8 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
*/
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
+ __this_cpu_sub(cpu_hw_events.n_metric, __this_cpu_read(cpu_hw_events.n_txn_metric));
+ __this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
perf_pmu_enable(pmu);
}
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 345442410a4d..6348105b6d30 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -235,6 +235,8 @@ struct cpu_hw_events {
they've never been enabled yet */
int n_txn; /* the # last events in the below arrays;
added in the current transaction */
+ int n_txn_metric;
+ int n_txn_pair;
int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
u64 tags[X86_PMC_IDX_MAX];
On 10/2/2020 7:02 AM, Peter Zijlstra wrote:
> On Wed, Sep 30, 2020 at 07:29:35AM -0700, [email protected] wrote:
>> From: Kan Liang <[email protected]>
>>
>> When a group that has TopDown members is failed to be scheduled, any
>> later TopDown groups will not return valid values.
>>
>> Here is an example.
>>
>> A background perf that occupies all the GP counters and the fixed
>> counter 1.
>> $perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
>> cycles,cycles}:D" -a
>>
>> A user monitors a TopDown group. It works well, because the fixed
>> counter 3 and the PERF_METRICS are available.
>> $perf stat -x, --topdown -- ./workload
>> retiring,bad speculation,frontend bound,backend bound,
>> 18.0,16.1,40.4,25.5,
>>
>> Then the user tries to monitor a group that has TopDown members.
>> Because of the cycles event, the group is failed to be scheduled.
>> $perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
>> topdown-fe-bound,topdown-bad-spec,cycles}'
>> -- ./workload
>> <not counted>,,slots,0,0.00,,
>> <not counted>,,topdown-retiring,0,0.00,,
>> <not counted>,,topdown-be-bound,0,0.00,,
>> <not counted>,,topdown-fe-bound,0,0.00,,
>> <not counted>,,topdown-bad-spec,0,0.00,,
>> <not counted>,,cycles,0,0.00,,
>>
>> The user tries to monitor a TopDown group again. It doesn't work anymore.
>> $perf stat -x, --topdown -- ./workload
>>
>> ,,,,,
>>
>> In a txn, cancel_txn() is to truncate the event_list for a canceled
>> group and update the number of events added in this transaction.
>> However, the number of TopDown events added in this transaction is not
>> updated. The kernel will probably fail to add new Topdown events.
>>
>> Check if the canceled group has Topdown events. If so, subtract the
>> TopDown events from n_metric accordingly.
>>
>> Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
>> Reported-by: Andi Kleen <[email protected]>
>> Reviewed-by: Andi Kleen <[email protected]>
>> Signed-off-by: Kan Liang <[email protected]>
>> ---
>> arch/x86/events/core.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index 0f3d01562ded..4cb3ccbe2d62 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
>> @@ -2017,6 +2017,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
>> {
>> unsigned int txn_flags;
>> struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> + int i;
>>
>> WARN_ON_ONCE(!cpuc->txn_flags); /* no txn in flight */
>>
>> @@ -2031,6 +2032,15 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
>> */
>> __this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
>> __this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
>> +
>> + /* Subtract Topdown events in the canceled group from n_metric */
>> + if (x86_pmu.intel_cap.perf_metrics && cpuc->n_metric) {
>> + for (i = 0; i < cpuc->n_txn; i++) {
>> + if (is_metric_event(cpuc->event_list[i + cpuc->n_events]))
>> + __this_cpu_dec(cpu_hw_events.n_metric);
>> + }
>> + WARN_ON_ONCE(__this_cpu_read(cpu_hw_events.n_metric) < 0);
>> + }
>> perf_pmu_enable(pmu);
>> }
>
>
> Urgh, I'd much rather we add n_txn_metric. But also, while looking at
> this, don't we have the same problem with n_pair ?
>
> Something like this perhaps...
>
Sure. For the perf metric, the below patch fixes the problem.
Tested-by: Kan Liang <[email protected]>
Thanks,
Kan
> ---
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 757e49755e7c..9b7792c0b6fb 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1066,6 +1066,7 @@ static int add_nr_metric_event(struct cpu_hw_events *cpuc,
> if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
> return -EINVAL;
> cpuc->n_metric++;
> + cpuc->n_txn_metric++;
> }
>
> return 0;
> @@ -1089,8 +1090,10 @@ static int collect_event(struct cpu_hw_events *cpuc, struct perf_event *event,
> return -EINVAL;
>
> cpuc->event_list[n] = event;
> - if (is_counter_pair(&event->hw))
> + if (is_counter_pair(&event->hw)) {
> cpuc->n_pair++;
> + cpuc->n_txn_pair++;
> + }
>
> return 0;
> }
> @@ -2062,6 +2065,8 @@ static void x86_pmu_start_txn(struct pmu *pmu, unsigned int txn_flags)
>
> perf_pmu_disable(pmu);
> __this_cpu_write(cpu_hw_events.n_txn, 0);
> + __this_cpu_write(cpu_hw_events.n_txn_metric, 0);
> + __this_cpu_write(cpu_hw_events.n_txn_pair, 0);
> }
>
> /*
> @@ -2087,6 +2092,8 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
> */
> __this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
> __this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
> + __this_cpu_sub(cpu_hw_events.n_metric, __this_cpu_read(cpu_hw_events.n_txn_metric));
> + __this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
> perf_pmu_enable(pmu);
> }
>
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 345442410a4d..6348105b6d30 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -235,6 +235,8 @@ struct cpu_hw_events {
> they've never been enabled yet */
> int n_txn; /* the # last events in the below arrays;
> added in the current transaction */
> + int n_txn_metric;
> + int n_txn_pair;
> int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
> u64 tags[X86_PMC_IDX_MAX];
>
>
On 10/2/20 8:16 AM, Liang, Kan wrote:
> On 10/2/2020 7:02 AM, Peter Zijlstra wrote:
>> On Wed, Sep 30, 2020 at 07:29:35AM -0700, [email protected] wrote:
>>> From: Kan Liang <[email protected]>
>>>
>>> When a group that has TopDown members is failed to be scheduled, any
>>> later TopDown groups will not return valid values.
>>>
>>> Here is an example.
>>>
>>> A background perf that occupies all the GP counters and the fixed
>>> counter 1.
>>> $perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
>>> cycles,cycles}:D" -a
>>>
>>> A user monitors a TopDown group. It works well, because the fixed
>>> counter 3 and the PERF_METRICS are available.
>>> $perf stat -x, --topdown -- ./workload
>>> retiring,bad speculation,frontend bound,backend bound,
>>> 18.0,16.1,40.4,25.5,
>>>
>>> Then the user tries to monitor a group that has TopDown members.
>>> Because of the cycles event, the group is failed to be scheduled.
>>> $perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
>>> topdown-fe-bound,topdown-bad-spec,cycles}'
>>> -- ./workload
>>> <not counted>,,slots,0,0.00,,
>>> <not counted>,,topdown-retiring,0,0.00,,
>>> <not counted>,,topdown-be-bound,0,0.00,,
>>> <not counted>,,topdown-fe-bound,0,0.00,,
>>> <not counted>,,topdown-bad-spec,0,0.00,,
>>> <not counted>,,cycles,0,0.00,,
>>>
>>> The user tries to monitor a TopDown group again. It doesn't work anymore.
>>> $perf stat -x, --topdown -- ./workload
>>>
>>> ,,,,,
>>>
>>> In a txn, cancel_txn() is to truncate the event_list for a canceled
>>> group and update the number of events added in this transaction.
>>> However, the number of TopDown events added in this transaction is not
>>> updated. The kernel will probably fail to add new Topdown events.
>>>
>>> Check if the canceled group has Topdown events. If so, subtract the
>>> TopDown events from n_metric accordingly.
>>>
>>> Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
>>> Reported-by: Andi Kleen <[email protected]>
>>> Reviewed-by: Andi Kleen <[email protected]>
>>> Signed-off-by: Kan Liang <[email protected]>
>>> ---
>>
>> Urgh, I'd much rather we add n_txn_metric. But also, while looking at
>> this, don't we have the same problem with n_pair ?
>>
>> Something like this perhaps...
>>
>
> Sure. For the perf metric, the below patch fixes the problem.
>
> Tested-by: Kan Liang <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Excerpt from test script:
sudo perf stat -e "{cycles,cycles,cycles,cycles}:D" -a sleep 10 &
# should succeed:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
# should fail:
sudo perf stat -e "{fp_ret_sse_avx_ops.all,fp_ret_sse_avx_ops.all,cycles}:D" -a workload
# previously failed, now succeeds with this patch:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
Thanks both,
Kim
On Fri, Oct 02, 2020 at 04:10:42PM -0500, Kim Phillips wrote:
> Tested-by: Kim Phillips <[email protected]>
---
Subject: perf/x86: Fix n_pair for cancelled txn
From: Peter Zijlstra <[email protected]>
Date: Mon Oct 5 10:09:06 CEST 2020
Kan reported that n_metric gets corrupted for cancelled transactions;
a similar issue exists for n_pair for AMD's Large Increment thing.
The problem was confirmed and confirmed fixed by Kim using:
sudo perf stat -e "{cycles,cycles,cycles,cycles}:D" -a sleep 10 &
# should succeed:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
# should fail:
sudo perf stat -e "{fp_ret_sse_avx_ops.all,fp_ret_sse_avx_ops.all,cycles}:D" -a workload
# previously failed, now succeeds with this patch:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
Fixes: 5738891229a2 ("perf/x86/amd: Add support for Large Increment per Cycle Events")
Reported-by: Kan Liang <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Tested-by: Kim Phillips <[email protected]>
---
arch/x86/events/core.c | 6 +++++-
arch/x86/events/perf_event.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1089,8 +1089,10 @@ static int collect_event(struct cpu_hw_e
return -EINVAL;
cpuc->event_list[n] = event;
- if (is_counter_pair(&event->hw))
+ if (is_counter_pair(&event->hw)) {
cpuc->n_pair++;
+ cpuc->n_txn_pair++;
+ }
return 0;
}
@@ -2062,6 +2064,7 @@ static void x86_pmu_start_txn(struct pmu
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_pair, 0);
}
/*
@@ -2087,6 +2090,7 @@ static void x86_pmu_cancel_txn(struct pm
*/
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
+ __this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
perf_pmu_enable(pmu);
}
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -235,6 +235,7 @@ struct cpu_hw_events {
they've never been enabled yet */
int n_txn; /* the # last events in the below arrays;
added in the current transaction */
+ int n_txn_pair;
int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
u64 tags[X86_PMC_IDX_MAX];
On Fri, Oct 02, 2020 at 09:16:11AM -0400, Liang, Kan wrote:
> Tested-by: Kan Liang <[email protected]>
---
Subject: perf/x86: Fix n_metric for cancelled txn
From: Peter Zijlstra <[email protected]>
Date: Mon Oct 5 10:10:24 CEST 2020
When a group that has TopDown members is failed to be scheduled, any
later TopDown groups will not return valid values.
Here is an example.
A background perf that occupies all the GP counters and the fixed
counter 1.
$perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
cycles,cycles}:D" -a
A user monitors a TopDown group. It works well, because the fixed
counter 3 and the PERF_METRICS are available.
$perf stat -x, --topdown -- ./workload
retiring,bad speculation,frontend bound,backend bound,
18.0,16.1,40.4,25.5,
Then the user tries to monitor a group that has TopDown members.
Because of the cycles event, the group is failed to be scheduled.
$perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
topdown-fe-bound,topdown-bad-spec,cycles}'
-- ./workload
<not counted>,,slots,0,0.00,,
<not counted>,,topdown-retiring,0,0.00,,
<not counted>,,topdown-be-bound,0,0.00,,
<not counted>,,topdown-fe-bound,0,0.00,,
<not counted>,,topdown-bad-spec,0,0.00,,
<not counted>,,cycles,0,0.00,,
The user tries to monitor a TopDown group again. It doesn't work anymore.
$perf stat -x, --topdown -- ./workload
,,,,,
In a txn, cancel_txn() is to truncate the event_list for a canceled
group and update the number of events added in this transaction.
However, the number of TopDown events added in this transaction is not
updated. The kernel will probably fail to add new Topdown events.
Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
Reported-by: Andi Kleen <[email protected]>
Reported-by: Kan Liang <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Tested-by: Kan Liang <[email protected]>
---
arch/x86/events/core.c | 3 +++
arch/x86/events/perf_event.h | 1 +
2 files changed, 4 insertions(+)
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1066,6 +1066,7 @@ static int add_nr_metric_event(struct cp
if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
return -EINVAL;
cpuc->n_metric++;
+ cpuc->n_txn_metric++;
}
return 0;
@@ -2065,6 +2066,7 @@ static void x86_pmu_start_txn(struct pmu
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
__this_cpu_write(cpu_hw_events.n_txn_pair, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_metric, 0);
}
/*
@@ -2091,6 +2093,7 @@ static void x86_pmu_cancel_txn(struct pm
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
+ __this_cpu_sub(cpu_hw_events.n_metric, __this_cpu_read(cpu_hw_events.n_txn_metric));
perf_pmu_enable(pmu);
}
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -236,6 +236,7 @@ struct cpu_hw_events {
int n_txn; /* the # last events in the below arrays;
added in the current transaction */
int n_txn_pair;
+ int n_txn_metric;
int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
u64 tags[X86_PMC_IDX_MAX];
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 3dbde69575637658d2094ee4416c21bc22eb89fe
Gitweb: https://git.kernel.org/tip/3dbde69575637658d2094ee4416c21bc22eb89fe
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 05 Oct 2020 10:10:24 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Tue, 06 Oct 2020 15:18:17 +02:00
perf/x86: Fix n_metric for cancelled txn
When a group that has TopDown members is failed to be scheduled, any
later TopDown groups will not return valid values.
Here is an example.
A background perf that occupies all the GP counters and the fixed
counter 1.
$perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
cycles,cycles}:D" -a
A user monitors a TopDown group. It works well, because the fixed
counter 3 and the PERF_METRICS are available.
$perf stat -x, --topdown -- ./workload
retiring,bad speculation,frontend bound,backend bound,
18.0,16.1,40.4,25.5,
Then the user tries to monitor a group that has TopDown members.
Because of the cycles event, the group is failed to be scheduled.
$perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
topdown-fe-bound,topdown-bad-spec,cycles}'
-- ./workload
<not counted>,,slots,0,0.00,,
<not counted>,,topdown-retiring,0,0.00,,
<not counted>,,topdown-be-bound,0,0.00,,
<not counted>,,topdown-fe-bound,0,0.00,,
<not counted>,,topdown-bad-spec,0,0.00,,
<not counted>,,cycles,0,0.00,,
The user tries to monitor a TopDown group again. It doesn't work anymore.
$perf stat -x, --topdown -- ./workload
,,,,,
In a txn, cancel_txn() is to truncate the event_list for a canceled
group and update the number of events added in this transaction.
However, the number of TopDown events added in this transaction is not
updated. The kernel will probably fail to add new Topdown events.
Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown metrics")
Reported-by: Andi Kleen <[email protected]>
Reported-by: Kan Liang <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Tested-by: Kan Liang <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/events/core.c | 3 +++
arch/x86/events/perf_event.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index a7248a3..7b802a7 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1041,6 +1041,7 @@ static int add_nr_metric_event(struct cpu_hw_events *cpuc,
if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
return -EINVAL;
cpuc->n_metric++;
+ cpuc->n_txn_metric++;
}
return 0;
@@ -2009,6 +2010,7 @@ static void x86_pmu_start_txn(struct pmu *pmu, unsigned int txn_flags)
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
__this_cpu_write(cpu_hw_events.n_txn_pair, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_metric, 0);
}
/*
@@ -2035,6 +2037,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
+ __this_cpu_sub(cpu_hw_events.n_metric, __this_cpu_read(cpu_hw_events.n_txn_metric));
perf_pmu_enable(pmu);
}
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 93e56d7..ee2b9b9 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -236,6 +236,7 @@ struct cpu_hw_events {
int n_txn; /* the # last events in the below arrays;
added in the current transaction */
int n_txn_pair;
+ int n_txn_metric;
int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
u64 tags[X86_PMC_IDX_MAX];
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 871a93b0aad65a7f44ee25f2d17932ef6d559850
Gitweb: https://git.kernel.org/tip/871a93b0aad65a7f44ee25f2d17932ef6d559850
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 05 Oct 2020 10:09:06 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Tue, 06 Oct 2020 15:18:17 +02:00
perf/x86: Fix n_pair for cancelled txn
Kan reported that n_metric gets corrupted for cancelled transactions;
a similar issue exists for n_pair for AMD's Large Increment thing.
The problem was confirmed and confirmed fixed by Kim using:
sudo perf stat -e "{cycles,cycles,cycles,cycles}:D" -a sleep 10 &
# should succeed:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
# should fail:
sudo perf stat -e "{fp_ret_sse_avx_ops.all,fp_ret_sse_avx_ops.all,cycles}:D" -a workload
# previously failed, now succeeds with this patch:
sudo perf stat -e "{fp_ret_sse_avx_ops.all}:D" -a workload
Fixes: 5738891229a2 ("perf/x86/amd: Add support for Large Increment per Cycle Events")
Reported-by: Kan Liang <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/events/core.c | 6 +++++-
arch/x86/events/perf_event.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index cb5cfef..a7248a3 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1064,8 +1064,10 @@ static int collect_event(struct cpu_hw_events *cpuc, struct perf_event *event,
return -EINVAL;
cpuc->event_list[n] = event;
- if (is_counter_pair(&event->hw))
+ if (is_counter_pair(&event->hw)) {
cpuc->n_pair++;
+ cpuc->n_txn_pair++;
+ }
return 0;
}
@@ -2006,6 +2008,7 @@ static void x86_pmu_start_txn(struct pmu *pmu, unsigned int txn_flags)
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_pair, 0);
}
/*
@@ -2031,6 +2034,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
*/
__this_cpu_sub(cpu_hw_events.n_added, __this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events, __this_cpu_read(cpu_hw_events.n_txn));
+ __this_cpu_sub(cpu_hw_events.n_pair, __this_cpu_read(cpu_hw_events.n_txn_pair));
perf_pmu_enable(pmu);
}
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 3454424..93e56d7 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -235,6 +235,7 @@ struct cpu_hw_events {
they've never been enabled yet */
int n_txn; /* the # last events in the below arrays;
added in the current transaction */
+ int n_txn_pair;
int assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
u64 tags[X86_PMC_IDX_MAX];