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

[Liang, Kan]

On 12/4/2020 6:27 PM, Jiri Olsa wrote:
On Mon, Nov 30, 2020 at 09:27:57AM -0800, kan.li...@linux.intel.com 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?


To keep the fmt consistent.

The patch divides the "printf(fmt,..." into two part. In the first half 
part, the symbol_conf.field_sep may be modified to " ";
In the second half part, we should not use the modified 
symbol_conf.field_sep for the below check. The field_sep keeps the 
original value and should be used.




+       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?


The phys addr is not the only member, the next patch (07/12) will add 
data page size as a new member.  So I factor out phys_addr in this and 
the previous patch (05/12).

Thanks,
Kan