Messages in this thread | | | Subject | Re: [PATCH V2 06/12] perf mem: Clean up output format | From | "Liang, Kan" <> | Date | Mon, 7 Dec 2020 15:19:06 -0500 |
| |
On 12/4/2020 6:27 PM, Jiri Olsa wrote: > On Mon, Nov 30, 2020 at 09:27:57AM -0800, kan.liang@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
| |