Skip to contentSkip to conversation

perf tools: Fix format value calculation

PATCH In review [email protected] 2 participants 4 messages Apr 4, 2016 – Apr 18, 2016 perf events

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 perf tools: Fix format value calculation

From: Kan Liang <[email protected]>

The calculation of format value also rely on the continuity of the format. However, uncore event format is not continuous. E.g. The bit 21 as qpi event is lost.

perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00, config2=0x3FE00/ -vvv
------------------------------------------------------------
perf_event_attr:

  type                             10
  size                             112
  config                           0x38

This patch checks the bit according to the bit position.

Signed-off-by Kan Liang <[email protected]>
---
 tools/perf/util/pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/util/pmu.c +3 -3
index bf34468..47c096c 100644
@@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head *formats, const char *name)
 static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
 			     bool zero)
 {
-	unsigned long fbit, vbit;
+	unsigned long fbit;
 
-	for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
+	for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
 
 		if (!test_bit(fbit, format))
 			continue;
 
-		if (value & (1llu << vbit++))
+		if (value & (1llu << fbit))
 			*v |= (1llu << fbit);
 		else if (zero)
 			*v &= ~(1llu << fbit);
Signature · 1 line
2.5.5
#1 lore raw
Quoted 6 lines · On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:

From: Kan Liang <[email protected]>

The calculation of format value also rely on the continuity of the format. However, uncore event format is not continuous. E.g. The bit 21 as qpi event is lost.

Quoted 7 lines

perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00, config2=0x3FE00/ -vvv
------------------------------------------------------------
perf_event_attr:

  type                             10
  size                             112
  config                           0x38

could you please share the event's format?

would be great to have some simple automated test for this one..

thanks,
jirka

Quoted 35 lines

This patch checks the bit according to the bit position.

Signed-off-by Kan Liang <[email protected]>
---
 tools/perf/util/pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/util/pmu.c +3 -3
index bf34468..47c096c 100644
@@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head *formats, const char *name)
 static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
 			     bool zero)
 {
-	unsigned long fbit, vbit;
+	unsigned long fbit;
 
-	for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
+	for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
 
 		if (!test_bit(fbit, format))
 			continue;
 
-		if (value & (1llu << vbit++))
+		if (value & (1llu << fbit))
 			*v |= (1llu << fbit);
 		else if (zero)
 			*v &= ~(1llu << fbit);
Signature · 1 line
2.5.5
author
#2 lore raw
Quoted 16 lines · On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:
Quoted 6 lines

From: Kan Liang <[email protected]>

The calculation of format value also rely on the continuity of the format. However, uncore event format is not continuous. E.g. The bit 21 as qpi event is lost.

Quoted 7 lines

perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00, config2=0x3FE00/ -vvv
------------------------------------------------------------
perf_event_attr:

  type                             10
  size                             112
  config                           0x38
Quoted 2 lines

could you please share the event's format?

cat /sys/devices/uncore_qpi_0/format/event
config:0-7,21

Quoted 2 lines

would be great to have some simple automated test for this one..

It looks there is a test case in perf test. 7: Test perf pmu format parsing
But it looks there are some issues for the test case.

The test format with config is

	{ "krava01", "config:0-1,62-63\n", },
	{ "krava02", "config:10-17\n", },
	{ "krava03", "config:5\n", },
The test input is
	{
		.config    = (char *) "krava01",
		.val.num   = 15,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},
	{
		.config    = (char *) "krava02",
		.val.num   = 170,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},
	{
		.config    = (char *) "krava03",
		.val.num   = 1,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},

The input value of "krava01" is 15 (0xf). The format of "krava01" is "config:0-1,62-63\n". Apparently, the input has wrong format. But it looks the test case doesn't think so. Also, at the end of the test case, it expects attr.config == 0xc00000000002a823. I think it doesn't make sense either.

Any thoughts?

Thanks,
Kan

Quoted 39 lines

thanks,
jirka

Quoted 15 lines

This patch checks the bit according to the bit position.

Signed-off-by Kan Liang <[email protected]>
---
 tools/perf/util/pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/util/pmu.c index renamed from tools/perf/util/pmu.c +0 -0

bf34468..47c096c 100644

tools/perf/util/pmu.c __u64 perf_pmu__format_bits(struct list_head · line 586

Quoted 1 line
tools/perf/util/pmu.c +0 -0
@@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head
 *formats, const char *name)  static void pmu_format_value(unsigned long

*format, __u64 value, __u64 *v,

Quoted 19 lines
tools/perf/util/pmu.c +3 -4
 			     bool zero)
 {
-	unsigned long fbit, vbit;
+	unsigned long fbit;
 
-	for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
+	for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
 
 		if (!test_bit(fbit, format))
 			continue;
 
-		if (value & (1llu << vbit++))
+		if (value & (1llu << fbit))
 			*v |= (1llu << fbit);
 		else if (zero)
 			*v &= ~(1llu << fbit);
--

2.5.5

author
#3 lore raw

Hi all,

There is confusion about the usage of format for me.

E.g. The event config is not continuous for uncore. cat /sys/devices/uncore_qpi_0/format/event config:0-7,21

I once thought that the user input should be
uncore_qpi_0/event=0x200038,..../
So I submitted this patch and previous patch
"ac0e2cd555373ae6f8f3a3ad3fbbf5b6d1e7aaaa"

But I took a deep look of the code, it looks current implementation expects the input as below. uncore_qpi_0/event=0x138,..../

I didn't find any documents about this case.
Could you please confirm about it?
If 0x138 is the expected input, I think we need to make it clear in the document.

Thanks,
Kan

Quoted 107 lines
Quoted 17 lines · On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:
Quoted 6 lines

From: Kan Liang <[email protected]>

The calculation of format value also rely on the continuity of the format. However, uncore event format is not continuous. E.g. The bit 21 as qpi event is lost.

Quoted 7 lines

perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00, config2=0x3FE00/ -vvv
------------------------------------------------------------
perf_event_attr:

  type                             10
  size                             112
  config                           0x38
Quoted 2 lines

could you please share the event's format?

cat /sys/devices/uncore_qpi_0/format/event
config:0-7,21

Quoted 2 lines

would be great to have some simple automated test for this one..

It looks there is a test case in perf test. 7: Test perf pmu format parsing
But it looks there are some issues for the test case.

The test format with config is

	{ "krava01", "config:0-1,62-63\n", },
	{ "krava02", "config:10-17\n", },
	{ "krava03", "config:5\n", },
The test input is
	{
		.config    = (char *) "krava01",
		.val.num   = 15,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},
	{
		.config    = (char *) "krava02",
		.val.num   = 170,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},
	{
		.config    = (char *) "krava03",
		.val.num   = 1,
		.type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
		.type_term = PARSE_EVENTS__TERM_TYPE_USER,
	},

The input value of "krava01" is 15 (0xf). The format of "krava01" is "config:0- 1,62-63\n".
Apparently, the input has wrong format. But it looks the test case doesn't think so.
Also, at the end of the test case, it expects attr.config == 0xc00000000002a823.
I think it doesn't make sense either.

Any thoughts?

Thanks,
Kan

Quoted 40 lines

thanks,
jirka

Quoted 16 lines

This patch checks the bit according to the bit position.

Signed-off-by Kan Liang <[email protected]>
---
 tools/perf/util/pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/util/pmu.c index renamed from tools/perf/util/pmu.c +0 -0

bf34468..47c096c 100644

tools/perf/util/pmu.c +0 -0
@@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head
 *formats, const char *name)  static void pmu_format_value(unsigned
Quoted 1 line

long

*format, __u64 value, __u64 *v,

Quoted 19 lines
tools/perf/util/pmu.c +3 -4
 			     bool zero)
 {
-	unsigned long fbit, vbit;
+	unsigned long fbit;
 
-	for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
+	for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
 
 		if (!test_bit(fbit, format))
 			continue;
 
-		if (value & (1llu << vbit++))
+		if (value & (1llu << fbit))
 			*v |= (1llu << fbit);
 		else if (zero)
 			*v &= ~(1llu << fbit);
--

2.5.5