This patch fixes a two bugs in the filter_events() function.
The first issue is that when shifting (compressing) the array
left, we need to also propagate the NULL termination,
otherwise we will have the same rightmost shifted event twice.
The second issue fixes the bug whereby if some mappings did not
exist, e.g., STALLED_CYCLES_FRONTEND, then any event after it
in the attrs array would disappear from the published list of
events. This could be verified easily on any system post
SNB (which do not publish STALLED_CYCLES_FRONTEND):
$ ./perf stat -e cycles,ref-cycles true
Performance counter stats for 'true':
1,217,348 cycles
<not supported> ref-cycles
The problem is that in filter_events() there is an assumption
that the argument (attrs) is organized in increasing continuous
event index related to the event_map(). But if we remove the
non-supported events by shifing the position in the array, then
the lookup x86_pmu.event_map() needs to compensate for it, otherwise
we are looking up at the wrong index. This patch corrects this problem
by compensating for the deleted events and with that ref-cycles reappears.
This problem was introduced into commit 8300daa26.
Fixes: 8300daa26 ("perf/x86: Filter out undefined events from sysfs events attribute")
Patch is relative to tip.git commit 03b6b9f.
Signed-off-by: Stephane Eranian <[email protected]>
---
arch/x86/kernel/cpu/perf_event.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index e7e63a9..70e16c7 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1534,6 +1534,7 @@ static void __init filter_events(struct attribute **attrs)
{
struct device_attribute *d;
struct perf_pmu_events_attr *pmu_attr;
+ int offset = 0;
int i, j;
for (i = 0; attrs[i]; i++) {
@@ -1542,14 +1543,23 @@ static void __init filter_events(struct attribute **attrs)
/* str trumps id */
if (pmu_attr->event_str)
continue;
- if (x86_pmu.event_map(i))
+ if (x86_pmu.event_map(i + offset))
continue;
for (j = i; attrs[j]; j++)
attrs[j] = attrs[j + 1];
+ attrs[j] = NULL;
/* Check the shifted attr. */
i--;
+
+ /*
+ * event_map() is index based, the attrs array is organized
+ * by increasing event index. If we shift the events, then
+ * we need to compensate for the event_map(), otherwise
+ * we are looking up the wrong event in the map
+ */
+ offset++;
}
}
--
2.5.0
On Sun, Dec 06, 2015 at 10:35:06PM +0100, Stephane Eranian wrote:
SNIP
>
> This problem was introduced into commit 8300daa26.
>
> Fixes: 8300daa26 ("perf/x86: Filter out undefined events from sysfs events attribute")
>
> Patch is relative to tip.git commit 03b6b9f.
>
> Signed-off-by: Stephane Eranian <[email protected]>
> ---
> arch/x86/kernel/cpu/perf_event.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index e7e63a9..70e16c7 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1534,6 +1534,7 @@ static void __init filter_events(struct attribute **attrs)
> {
> struct device_attribute *d;
> struct perf_pmu_events_attr *pmu_attr;
> + int offset = 0;
> int i, j;
>
> for (i = 0; attrs[i]; i++) {
> @@ -1542,14 +1543,23 @@ static void __init filter_events(struct attribute **attrs)
> /* str trumps id */
> if (pmu_attr->event_str)
> continue;
> - if (x86_pmu.event_map(i))
> + if (x86_pmu.event_map(i + offset))
> continue;
>
> for (j = i; attrs[j]; j++)
> attrs[j] = attrs[j + 1];
> + attrs[j] = NULL;
hum.. the loop above quits when attrs[j] == NULL,
so I dont think u need to assign it again after
the NULL should get shifted in the last iteration of the loop above
>
> /* Check the shifted attr. */
> i--;
> +
> + /*
> + * event_map() is index based, the attrs array is organized
> + * by increasing event index. If we shift the events, then
> + * we need to compensate for the event_map(), otherwise
> + * we are looking up the wrong event in the map
> + */
> + offset++;
ouch, thanks for fixing this
I tested the patch and it fixes the issue
jirka