[PATCH] perf headers: fix processing of pmu_mappings

[PATCH] perf headers: fix processing of pmu_mappings

[Stephane Eranian]

This patch fixes a bug in process_pmu_mappings() where the code
would not produce an env->pmu_mappings string that was easily parsable.
The function parses the PMU_MAPPING header information into a string
consisting of value:name pairs where value is the PMU type identifier
and name is the PMU name, e.g., 10:ibs_fetch. As it was, the code
was producing a truncated string with only the first pair showing
even though the rest was there but after the \0.
This patch fixes the problem byt adding a proper white space between
pairs and moving the \0 termination to the end. With this patch applied,
all pairs appear and are easily parsed.

Before:
14:amd_iommu_1

After:
14:amd_iommu_1 7:uprobe 5:breakpoint 10:amd_l3 19:amd_iommu_6 8:power 4:cpu 17:amd_iommu_4 15:amd_iommu_2 1:software 6:kprobe 13:amd_iommu_0 9:amd_df 20:amd_iommu_7 18:amd_iommu_5 2:tracepoint 21:msr 12:ibs_op 16:amd_iommu_3 11:ibs_fetch

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/header.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 7a67d017d72c3..cf72124da9350 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2462,13 +2462,15 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
 static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
 {
 	char *name;
-	u32 pmu_num;
+	u32 pmu_num, o_num;
 	u32 type;
 	struct strbuf sb;
 
 	if (do_read_u32(ff, &pmu_num))
 		return -1;
 
+	o_num = pmu_num;
+
 	if (!pmu_num) {
 		pr_debug("pmu mappings not available\n");
 		return 0;
@@ -2486,10 +2488,11 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
 		if (!name)
 			goto error;
 
-		if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
+		/* add proper spacing between entries */
+		if (pmu_num < o_num && strbuf_add(&sb, " ", 1) < 0)
 			goto error;
-		/* include a NULL character at the end */
-		if (strbuf_add(&sb, "", 1) < 0)
+
+		if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
 			goto error;
 
 		if (!strcmp(name, "msr"))
@@ -2498,6 +2501,9 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
 		free(name);
 		pmu_num--;
 	}
+	/* include a NULL character at the end */
+	if (strbuf_add(&sb, "", 1) < 0)
+		goto error;
 	ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
 	return 0;
 
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH] perf headers: fix processing of pmu_mappings

[Ian Rogers]

On Mon, Jun 8, 2020 at 9:18 AM Stephane Eranian <eranian@google.com> wrote:
>
> This patch fixes a bug in process_pmu_mappings() where the code
> would not produce an env->pmu_mappings string that was easily parsable.
> The function parses the PMU_MAPPING header information into a string
> consisting of value:name pairs where value is the PMU type identifier
> and name is the PMU name, e.g., 10:ibs_fetch. As it was, the code
> was producing a truncated string with only the first pair showing
> even though the rest was there but after the \0.
> This patch fixes the problem byt adding a proper white space between
> pairs and moving the \0 termination to the end. With this patch applied,
> all pairs appear and are easily parsed.
>
> Before:
> 14:amd_iommu_1
>
> After:
> 14:amd_iommu_1 7:uprobe 5:breakpoint 10:amd_l3 19:amd_iommu_6 8:power 4:cpu 17:amd_iommu_4 15:amd_iommu_2 1:software 6:kprobe 13:amd_iommu_0 9:amd_df 20:amd_iommu_7 18:amd_iommu_5 2:tracepoint 21:msr 12:ibs_op 16:amd_iommu_3 11:ibs_fetch
>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/util/header.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 7a67d017d72c3..cf72124da9350 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2462,13 +2462,15 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
>  static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>  {
>         char *name;
> -       u32 pmu_num;
> +       u32 pmu_num, o_num;
>         u32 type;
>         struct strbuf sb;
>
>         if (do_read_u32(ff, &pmu_num))
>                 return -1;
>
> +       o_num = pmu_num;
> +
>         if (!pmu_num) {
>                 pr_debug("pmu mappings not available\n");
>                 return 0;
> @@ -2486,10 +2488,11 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>                 if (!name)
>                         goto error;
>
> -               if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
> +               /* add proper spacing between entries */
> +               if (pmu_num < o_num && strbuf_add(&sb, " ", 1) < 0)
>                         goto error;
> -               /* include a NULL character at the end */
> -               if (strbuf_add(&sb, "", 1) < 0)
> +
> +               if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
>                         goto error;
>
>                 if (!strcmp(name, "msr"))
> @@ -2498,6 +2501,9 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>                 free(name);
>                 pmu_num--;
>         }
> +       /* include a NULL character at the end */
> +       if (strbuf_add(&sb, "", 1) < 0)
> +               goto error;
>         ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
>         return 0;
>
> --
> 2.27.0.278.ge193c7cf3a9-goog

Acked-by: Ian Rogers <irogers@google.com>

A lot of the complexity in this code came from strbuf not \0
terminating strings. Would a strbuf that always \0 terminated be a
useful change? In general there's a lack of consistent style with
strbuf, strcat and asprintf being used in different parts of the code.
Perhaps strbuf use should migrate to asprintf? There are currently
just 13 callers of strbuf_init.

Thanks,
Ian

Re: [PATCH] perf headers: fix processing of pmu_mappings

[Arnaldo Carvalho de Melo]

Em Mon, Jun 08, 2020 at 09:52:43AM -0700, Ian Rogers escreveu:
> On Mon, Jun 8, 2020 at 9:18 AM Stephane Eranian <eranian@google.com> wrote:
> A lot of the complexity in this code came from strbuf not \0
> terminating strings. Would a strbuf that always \0 terminated be a
> useful change? In general there's a lack of consistent style with
> strbuf, strcat and asprintf being used in different parts of the code.
> Perhaps strbuf use should migrate to asprintf? There are currently

I think that going to asprintf is best, this came from long ago, when
perf reused parts of the git sources as a starting point.

> just 13 callers of strbuf_init.

- Arnaldo

Re: [PATCH] perf headers: fix processing of pmu_mappings

[Arnaldo Carvalho de Melo]

Em Mon, Jun 08, 2020 at 09:52:43AM -0700, Ian Rogers escreveu:
> On Mon, Jun 8, 2020 at 9:18 AM Stephane Eranian <eranian@google.com> wrote:
> > This patch fixes a bug in process_pmu_mappings() where the code
> > would not produce an env->pmu_mappings string that was easily parsable.
> > The function parses the PMU_MAPPING header information into a string
> > consisting of value:name pairs where value is the PMU type identifier
> > and name is the PMU name, e.g., 10:ibs_fetch. As it was, the code
> > was producing a truncated string with only the first pair showing
> > even though the rest was there but after the \0.
> > This patch fixes the problem byt adding a proper white space between
> > pairs and moving the \0 termination to the end. With this patch applied,
> > all pairs appear and are easily parsed.

> > Before:
> > 14:amd_iommu_1

> > After:
> > 14:amd_iommu_1 7:uprobe 5:breakpoint 10:amd_l3 19:amd_iommu_6 8:power 4:cpu 17:amd_iommu_4 15:amd_iommu_2 1:software 6:kprobe 13:amd_iommu_0 9:amd_df 20:amd_iommu_7 18:amd_iommu_5 2:tracepoint 21:msr 12:ibs_op 16:amd_iommu_3 11:ibs_fetch

Please check print_pmu_mappings() in tools/perf/util/header.c

Before your patch:

  [root@five ~]# perf report --header-only | grep "pmu mappings"
  # pmu mappings: amd_df = 8, software = 1, ibs_op = 11, ibs_fetch = 10, uprobe = 7, cpu = 4, amd_iommu_0 = 12, breakpoint = 5, amd_l3 = 9, tracepoint = 2, kprobe = 6, msr = 13
  [root@five ~]#

After your patch:

  [root@five ~]# perf report --header-only | grep "pmu mappings"
  # pmu mappings: amd_df 1:software 11:ibs_op 10:ibs_fetch 7:uprobe 4:cpu 12:amd_iommu_0 5:breakpoint 9:amd_l3 2:tracepoint 6:kprobe 13:msr = 8# pmu mappings: unable to read
  [root@five ~]# 

I think having it space separated, as you propose, is best, but haven't
checked if there are other cases that process ff->ph->env.pmu_mappings
and expect it to be \0 separated, like print_pmu_mappings().

Regards,

- Arnaldo