Re: [PATCH V2 06/12] perf mem: Clean up output format

From: Jiri Olsa
Date: Fri Dec 04 2020 - 18:31:00 EST


On Mon, Nov 30, 2020 at 09:27:57AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:

SNIP

> @@ -172,7 +172,7 @@ dump_raw_samples(struct perf_tool *tool,
> {
> struct perf_mem *mem = container_of(tool, struct perf_mem, tool);
> struct addr_location al;
> - const char *fmt;
> + const char *fmt, *field_sep;
>
> if (machine__resolve(machine, &al, sample) < 0) {
> fprintf(stderr, "problem processing %d event, skipping it.\n",
> @@ -186,60 +186,41 @@ dump_raw_samples(struct perf_tool *tool,
> if (al.map != NULL)
> al.map->dso->hit = 1;
>
> - if (mem->phys_addr) {
> - if (symbol_conf.field_sep) {
> - fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s0x%016"PRIx64
> - "%s%"PRIu64"%s0x%"PRIx64"%s%s:%s\n";
> - } else {
> - fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64
> - "%s0x%016"PRIx64"%s%5"PRIu64"%s0x%06"PRIx64
> - "%s%s:%s\n";
> - symbol_conf.field_sep = " ";
> - }
> -
> - printf(fmt,
> - sample->pid,
> - symbol_conf.field_sep,
> - sample->tid,
> - symbol_conf.field_sep,
> - sample->ip,
> - symbol_conf.field_sep,
> - sample->addr,
> - symbol_conf.field_sep,
> - sample->phys_addr,
> - symbol_conf.field_sep,
> - sample->weight,
> - symbol_conf.field_sep,
> - sample->data_src,
> - symbol_conf.field_sep,
> - al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> - al.sym ? al.sym->name : "???");
> + field_sep = symbol_conf.field_sep;

hum, what's the point of having field_sep?

> + if (field_sep) {
> + fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s";
> } else {
> - if (symbol_conf.field_sep) {
> - fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s%"PRIu64
> - "%s0x%"PRIx64"%s%s:%s\n";
> - } else {
> - fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64
> - "%s%5"PRIu64"%s0x%06"PRIx64"%s%s:%s\n";
> - symbol_conf.field_sep = " ";
> - }
> + fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64"%s";
> + symbol_conf.field_sep = " ";
> + }
> + printf(fmt,
> + sample->pid,
> + symbol_conf.field_sep,
> + sample->tid,
> + symbol_conf.field_sep,
> + sample->ip,
> + symbol_conf.field_sep,
> + sample->addr,
> + symbol_conf.field_sep);
>
> - printf(fmt,
> - sample->pid,
> - symbol_conf.field_sep,
> - sample->tid,
> - symbol_conf.field_sep,
> - sample->ip,
> - symbol_conf.field_sep,
> - sample->addr,
> - symbol_conf.field_sep,
> - sample->weight,
> - symbol_conf.field_sep,
> - sample->data_src,
> - symbol_conf.field_sep,
> - al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> - al.sym ? al.sym->name : "???");
> + if (mem->phys_addr) {
> + printf("0x%016"PRIx64"%s",
> + sample->phys_addr,
> + symbol_conf.field_sep);
> }
> +
> + if (field_sep)
> + fmt = "%"PRIu64"%s0x%"PRIx64"%s%s:%s\n";
> + else
> + fmt = "%5"PRIu64"%s0x%06"PRIx64"%s%s:%s\n";
> +
> + printf(fmt,
> + sample->weight,
> + symbol_conf.field_sep,
> + sample->data_src,
> + symbol_conf.field_sep,
> + al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> + al.sym ? al.sym->name : "???");
> out_put:
> addr_location__put(&al);
> return 0;
> @@ -287,10 +268,12 @@ static int report_raw_events(struct perf_mem *mem)
> if (ret < 0)
> goto out_delete;
>
> + printf("# PID, TID, IP, ADDR, ");
> +
> if (mem->phys_addr)
> - printf("# PID, TID, IP, ADDR, PHYS ADDR, LOCAL WEIGHT, DSRC, SYMBOL\n");
> - else
> - printf("# PID, TID, IP, ADDR, LOCAL WEIGHT, DSRC, SYMBOL\n");
> + printf("PHYS ADDR, ");
> +
> + printf("LOCAL WEIGHT, DSRC, SYMBOL\n");

if phys addr is the only member, can't we just squeeze it via
"%s", mem->phys_addr ? "PHYS ADDR" : "",
like I mentioned in the other reply? and same also above?

thanks,
jirka

>
> ret = perf_session__process_events(session);
>
> --
> 2.17.1
>