perf: header: Regression

PATCH In review Andrew Jones 2 participants 12 messages Oct 22, 2012 – Nov 14, 2012 perf events

Share & tools

Andrew Jones

author

2012-10-22 13:11 UTC #0 lore raw

The patch series that contains 7e94cfcc9d20 regresses the header output. Below is a diff of info from before and after the series

58,64c58,59
< # node1 meminfo : total = 16777216 kB, free = 15712440 kB
< # node1 cpu list : 2,6,10,14,18,22,26,30
< # node2 meminfo : total = 16777216 kB, free = 16299132 kB
< # node2 cpu list : 1,5,9,13,17,21,25,29
< # node3 meminfo : total = 16777216 kB, free = 16217192 kB
< # node3 cpu list : 3,7,11,15,19,23,27,31
< # pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 = 15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19, uncore_mbox_1 = 20, uncore_rbox_0 = 21, uncore_rbox_1 = 22, uncore_sbox_0 = 17, uncore_sbox_1 = 18, breakpoint = 5, uncore_ubox = 6, uncore_wbox = 23
---

Quoted 2 lines

# numa topology : not available
# pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 =

15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19# pmu
mappings: unable to read

As can be seen in the diff, before the series we had complete numa info and pmu mappings info. After we get

"# numa topology : not available" and "# pmu
mappings: unable to read"

Drew

Namhyung Kim

maintainer

2012-10-23 02:07 UTC #1 lore raw

Hi Andrew,

Quoted 26 lines · Andrew Jones

The patch series that contains 7e94cfcc9d20 regresses the header output. Below is a diff of info from before and after the series
58,64c58,59
< # node1 meminfo : total = 16777216 kB, free = 15712440 kB
< # node1 cpu list : 2,6,10,14,18,22,26,30
< # node2 meminfo : total = 16777216 kB, free = 16299132 kB
< # node2 cpu list : 1,5,9,13,17,21,25,29
< # node3 meminfo : total = 16777216 kB, free = 16217192 kB
< # node3 cpu list : 3,7,11,15,19,23,27,31
< # pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 = 15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19, uncore_mbox_1 = 20, uncore_rbox_0 = 21, uncore_rbox_1 = 22, uncore_sbox_0 = 17, uncore_sbox_1 = 18, breakpoint = 5, uncore_ubox = 6, uncore_wbox = 23
---
Quoted 2 lines
# numa topology : not available
# pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 =
15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19# pmu
mappings: unable to read

Quoted 5 lines

As can be seen in the diff, before the series we had complete numa info and pmu mappings info. After we get
"# numa topology : not available" and "# pmu
mappings: unable to read"

Hmm.. I couldn't reproduce on my system:

# node0 meminfo  : total = 33505624 kB, free = 25213528 kB
# node0 cpu list : 0-11
# pmu mappings: cpu = 4, software = 1, uncore_pcu = 13, tracepoint = 2,
uncore_imc_0 = 15, uncore_imc_1 = 16, uncore_imc_2 = 17, uncore_imc_3
= 18, uncore_qpi_0 = 19, u = 20, uncore_cbox_0 = 7, uncore_cbox_1 = 8,
uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, un = 12,
uncore_ha = 14, uncore_r2pcie = 21, uncore_r3qpi_0 = 22,
uncore_r3qpi_1 = 23, breakpoint = 5, uncore_ubox = 6

I don't have a NUMA machine. Could you send your perf.data file or a link via private mail?

Thanks,
Namhyung

Andrew Jones

author

2012-10-23 07:27 UTC #2 lore raw

Quoted 46 lines · Namhyung Kim

Hi Andrew,
Quoted 26 lines · Andrew Jones
The patch series that contains 7e94cfcc9d20 regresses the header output. Below is a diff of info from before and after the series
58,64c58,59
< # node1 meminfo : total = 16777216 kB, free = 15712440 kB
< # node1 cpu list : 2,6,10,14,18,22,26,30
< # node2 meminfo : total = 16777216 kB, free = 16299132 kB
< # node2 cpu list : 1,5,9,13,17,21,25,29
< # node3 meminfo : total = 16777216 kB, free = 16217192 kB
< # node3 cpu list : 3,7,11,15,19,23,27,31
< # pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 = 15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19, uncore_mbox_1 = 20, uncore_rbox_0 = 21, uncore_rbox_1 = 22, uncore_sbox_0 = 17, uncore_sbox_1 = 18, breakpoint = 5, uncore_ubox = 6, uncore_wbox = 23
---
Quoted 2 lines
# numa topology : not available
# pmu mappings: cpu = 4, software = 1, tracepoint = 2, uncore_bbox_0 =
15, uncore_bbox_1 = 16, uncore_cbox_0 = 7, uncore_cbox_1 = 8, uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, uncore_cbox_5 = 12, uncore_cbox_6 = 13, uncore_cbox_7 = 14, uncore_mbox_0 = 19# pmu
mappings: unable to read
Quoted 5 lines
As can be seen in the diff, before the series we had complete numa info and pmu mappings info. After we get
"# numa topology : not available" and "# pmu
mappings: unable to read"
Hmm.. I couldn't reproduce on my system:
# node0 meminfo  : total = 33505624 kB, free = 25213528 kB
# node0 cpu list : 0-11
# pmu mappings: cpu = 4, software = 1, uncore_pcu = 13, tracepoint = 2,
uncore_imc_0 = 15, uncore_imc_1 = 16, uncore_imc_2 = 17, uncore_imc_3
= 18, uncore_qpi_0 = 19, u = 20, uncore_cbox_0 = 7, uncore_cbox_1 = 8,
uncore_cbox_2 = 9, uncore_cbox_3 = 10, uncore_cbox_4 = 11, un = 12,
uncore_ha = 14, uncore_r2pcie = 21, uncore_r3qpi_0 = 22,
uncore_r3qpi_1 = 23, breakpoint = 5, uncore_ubox = 6 

Quoted 2 lines

I don't have a NUMA machine. Could you send your perf.data file or a link via private mail?

I sent a link to my perf.data in a private mail. Let me know if you'd like me to test a fix.

Thanks,
drew

Quoted 3 lines

Thanks,
Namhyung

Namhyung Kim

maintainer

2012-10-23 13:46 UTC #4 lore raw

PATCH 2/2 perf tools: Fix strbuf_addf() when the buffer needs to grow

In reply to Namhyung Kim

This was found during chasing down the header output regression. The strbuf_addf() was checking buffer length with a result of vscnprintf() which cannot be greater than that of strbuf_avail().

Since numa topology and pmu mapping info in header were converted to use strbuf, it sometimes caused uninteresting behaviors with the broken strbuf.

Fix it by using vsnprintf() which returns desired output string length regardless of the available buffer size and grow the buffer if needed.

Reported-by Andrew Jones <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/strbuf.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/perf/util/strbuf.c +4 -4

index 2eeb51baf077..cfa906882e2c 100644
@@ -90,17 +90,17 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
 	va_start(ap, fmt);
-	len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 	va_end(ap);
 	if (len < 0)
-		die("your vscnprintf is broken");
+		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		va_start(ap, fmt);
-		len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 		va_end(ap);
 		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your snprintf is broken");
+			die("this should not happen, your vsnprintf is broken");
 		}
 	}
 	strbuf_setlen(sb, sb->len + len);

Signature · 1 line

1.7.9.2

Namhyung Kim

maintainer

2012-10-23 13:46 UTC #3 lore raw

PATCH 1/2 perf header: Fix numa topology printing

In reply to Andrew Jones

Andrew reported that the commit 7e94cfcc9d20 ("perf header: Use pre- processed session env when printing") regresses the header output. It was because of a missed string pointer calculation in the loop.

Reported-by Andrew Jones <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/header.c |    2 ++
 1 file changed, 2 insertions(+)

tools/perf/util/header.c +2 -0

index 7daad237dea5..566b84c695c8 100644
@@ -1378,6 +1378,8 @@ static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
 
 		str = tmp + 1;
 		fprintf(fp, "# node%u cpu list : %s\n", c, str);
+
+		str += strlen(str) + 1;
 	}
 	return;
 error:

Signature · 1 line

1.7.9.2

Andrew Jones

author Tested-by on v1 1/2

2012-10-23 14:12 UTC #7 lore raw

Re: [PATCH 1/2] perf header: Fix numa topology printing

Quoted 26 lines · Namhyung Kim

Andrew reported that the commit 7e94cfcc9d20 ("perf header: Use pre- processed session env when printing") regresses the header output. It was because of a missed string pointer calculation in the loop.
Reported-by Andrew Jones <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/header.c |    2 ++
 1 file changed, 2 insertions(+)
tools/perf/util/header.c +2 -0
index 7daad237dea5..566b84c695c8 100644
@@ -1378,6 +1378,8 @@ static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
 
 		str = tmp + 1;
 		fprintf(fp, "# node%u cpu list : %s\n", c, str);
+
+		str += strlen(str) + 1;
 	}
 	return;
 error:
Signature · 1 line
1.7.9.2

works for me

Tested-by Andrew Jones <[email protected]>

Namhyung Kim

maintainer

2012-10-24 07:00 UTC #8 lore raw

Re: [PATCH 1/2] perf header: Fix numa topology printing

Hi Andrew,

Quoted 8 lines · Andrew Jones

Quoted 3 lines · Namhyung Kim
Andrew reported that the commit 7e94cfcc9d20 ("perf header: Use pre- processed session env when printing") regresses the header output. It was because of a missed string pointer calculation in the loop.
works for me
Tested-by Andrew Jones <[email protected]>

Did you test both of the patches?

Thanks,
Namhyung

Andrew Jones

author

2012-10-24 07:49 UTC #9 lore raw

Re: [PATCH 1/2] perf header: Fix numa topology printing

Quoted 8 lines · Namhyung Kim

Hi Andrew,
Quoted 5 lines · Andrew Jones
Quoted 3 lines · Namhyung Kim
Andrew reported that the commit 7e94cfcc9d20 ("perf header: Use pre- processed session env when printing") regresses the header output. It was because of a missed string pointer calculation in the loop.

Quoted 5 lines

Quoted 3 lines
works for me
Tested-by Andrew Jones <[email protected]>
Did you test both of the patches?

Yes, I applied both before testing. I'll add a tested by to the other as well.

drew

Quoted 3 lines

Thanks,
Namhyung

Andrew Jones

author Tested-by on v1 2/2

2012-10-24 07:49 UTC #5 lore raw

Re: [PATCH 2/2] perf tools: Fix strbuf_addf() when the buffer needs to grow

In reply to Namhyung Kim

Quoted 47 lines · Namhyung Kim

This was found during chasing down the header output regression. The strbuf_addf() was checking buffer length with a result of vscnprintf() which cannot be greater than that of strbuf_avail().
Since numa topology and pmu mapping info in header were converted to use strbuf, it sometimes caused uninteresting behaviors with the broken strbuf.
Fix it by using vsnprintf() which returns desired output string length regardless of the available buffer size and grow the buffer if needed.
Reported-by Andrew Jones <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/strbuf.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/strbuf.c +4 -4
index 2eeb51baf077..cfa906882e2c 100644
@@ -90,17 +90,17 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
 	va_start(ap, fmt);
-	len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 	va_end(ap);
 	if (len < 0)
-		die("your vscnprintf is broken");
+		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		va_start(ap, fmt);
-		len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 		va_end(ap);
 		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your snprintf is broken");
+			die("this should not happen, your vsnprintf is broken");
 		}
 	}
 	strbuf_setlen(sb, sb->len + len);
Signature · 1 line
1.7.9.2

Tested-by Andrew Jones <[email protected]>

Namhyung Kim

maintainer

2012-10-29 15:18 UTC #10 lore raw

Re: [PATCH 1/2] perf header: Fix numa topology printing

In reply to Andrew Jones

2012-10-24 (수), 09:49 +0200, Andrew Jones:

Quoted 7 lines · Namhyung Kim

Quoted 6 lines · Andrew Jones
Quoted 5 lines · Namhyung Kim
Quoted 3 lines
Andrew reported that the commit 7e94cfcc9d20 ("perf header: Use pre- processed session env when printing") regresses the header output. It was because of a missed string pointer calculation in the loop.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
works for me
Tested-by Andrew Jones <[email protected]>
Did you test both of the patches?
Yes, I applied both before testing. I'll add a tested by to the other as well.

Arnaldo, I think these two should go through the urgent branch. Would you apply them?

Thanks,
Namhyung

tip-bot for Namhyung Kim

maintainer

2012-11-14 06:27 UTC #11 lore raw

[tip:perf/urgent] perf header: Fix numa topology printing

In reply to Namhyung Kim

Commit-ID:  1234471e2d11e4ee47f6de452dd8b8aeb09b45de
Gitweb:     http://git.kernel.org/tip/1234471e2d11e4ee47f6de452dd8b8aeb09b45de
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 23 Oct 2012 22:44:49 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 30 Oct 2012 10:32:55 -0200

perf header: Fix numa topology printing

Reported-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

Cc Andrew Jones <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/header.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

tools/perf/util/header.c +2 -0

index 7daad23..566b84c 100644
@@ -1378,6 +1378,8 @@ static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
 
 		str = tmp + 1;
 		fprintf(fp, "# node%u cpu list : %s\n", c, str);
+
+		str += strlen(str) + 1;
 	}
 	return;
 error:

tip-bot for Namhyung Kim

maintainer

2012-11-14 06:28 UTC #6 lore raw

[tip:perf/urgent] perf tools: Fix strbuf_addf() when the buffer needs to grow

In reply to Namhyung Kim

Commit-ID:  f787d9519fb10411f2948f5b9957a1669879ba84
Gitweb:     http://git.kernel.org/tip/f787d9519fb10411f2948f5b9957a1669879ba84
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 23 Oct 2012 22:44:50 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 30 Oct 2012 10:32:56 -0200

perf tools: Fix strbuf_addf() when the buffer needs to grow

This was found during chasing down the header output regression. The strbuf_addf() was checking buffer length with a result of vscnprintf() which cannot be greater than that of strbuf_avail().

Since numa topology and pmu mapping info in header were converted to use strbuf, it sometimes caused uninteresting behaviors with the broken strbuf.

Fix it by using vsnprintf() which returns desired output string length regardless of the available buffer size and grow the buffer if needed.

Reported-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

Cc Andrew Jones <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/strbuf.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

tools/perf/util/strbuf.c +4 -4

index 2eeb51b..cfa9068 100644
@@ -90,17 +90,17 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
 	va_start(ap, fmt);
-	len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 	va_end(ap);
 	if (len < 0)
-		die("your vscnprintf is broken");
+		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		va_start(ap, fmt);
-		len = vscnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 		va_end(ap);
 		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your snprintf is broken");
+			die("this should not happen, your vsnprintf is broken");
 		}
 	}
 	strbuf_setlen(sb, sb->len + len);