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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <978f0cb9-43d6-1ae5-e1ce-5ec4cc9fca12@linux.intel.com>
Date:   Mon, 7 Dec 2020 15:19:06 -0500
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     acme@...nel.org, mingo@...nel.org, linux-kernel@...r.kernel.org,
        namhyung@...nel.org, eranian@...gle.com, ak@...ux.intel.com,
        mark.rutland@....com, will@...nel.org, mpe@...erman.id.au
Subject: Re: [PATCH V2 06/12] perf mem: Clean up output format



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