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


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