| 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
| ||
|
Message-ID: <316f99df-257a-c981-1a44-464c797e56bb@linux.intel.com>
Date: Mon, 28 Jan 2019 13:41:04 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: peterz@...radead.org, acme@...nel.org, tglx@...utronix.de,
mingo@...hat.com, linux-kernel@...r.kernel.org, eranian@...gle.com,
namhyung@...nel.org, ak@...ux.intel.com
Subject: Re: [PATCH V2 05/12] perf mem: Clean up output format and sort order
string
On 1/28/2019 10:09 AM, Jiri Olsa wrote:
> On Wed, Jan 23, 2019 at 07:15:23AM -0800, kan.liang@...ux.intel.com wrote:
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> Now, "--phys-data" is the only option which impacts the output format
>> and sort order. A simple "if else" is enough to handle the option.
>> But there will be more options added, e.g. "--data-page-size", which
>> also impact the output format and sort order. The code will become too
>> complex to be maintained.
>>
>> Divide the big printf into several small pieces. Output the specific
>> piece only if the related option is applied.
>>
>> Divide the big sort order string into several small pieces as well.
>> Appends specific sort string only if the related option is applied.
>>
>> No functional change.
>
>
> hum, I'm getting some functional changes:
>
> # perf mem record
> # perf.old mem report --stdio > base
> # perf mem report --stdio > new
> # diff -puw base new
> --- base 2019-01-28 16:06:13.264991170 +0100
> +++ new 2019-01-28 16:06:31.347680820 +0100
> @@ -5,8133 +5,7346 @@
> #
> # Samples: 3K of event 'cpu/mem-loads,ldlat=30/P'
> # Total weight : 203677
> -# Sort order : local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked
> +# Sort order : mem,sym,dso,symbol_daddr,dso_daddr,tlb,locked,ipc_null
> #
> -# Overhead Samples Local Weight Memory access Symbol Shared Object Data Symbol Data Object Snoop TLB access Locked
> -# ........ ............ ............ ........................ ......................................... ................ ................................................. ............................. ............ ...................... ......
> +# Overhead Samples Memory access Symbol Shared Object Data Symbol Data Object TLB access Locked IPC [IPC Coverage]
> +# ........ ............ ........................ ......................................... ................ ................................................. ............................. ...................... ...... ....................
>
>
> could you please split this patch to separate simple changes?
>
Thanks for the test. I will fix it and split the patch in V3.
Thanks,
Kan
> jirka
>
>
>>
>> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
>> ---
>>
>> No changes since V1
>>
>> tools/perf/builtin-mem.c | 132 +++++++++++++++++++++++------------------------
>> tools/perf/util/sort.h | 2 +
>> 2 files changed, 66 insertions(+), 68 deletions(-)
>>
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 57393e9..6048fca 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -14,6 +14,7 @@
>> #include "util/mem-events.h"
>> #include "util/debug.h"
>> #include "util/symbol.h"
>> +#include "util/sort.h"
>>
>> #define MEM_OPERATION_LOAD 0x1
>> #define MEM_OPERATION_STORE 0x2
>> @@ -153,7 +154,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",
>> @@ -167,60 +168,45 @@ 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;
>> + 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);
>>
>> + if (mem->phys_addr) {
>> + if (field_sep)
>> + fmt = "0x%"PRIx64"%s";
>> + else
>> + fmt = "0x%016"PRIx64"%s";
>> 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 : "???");
>> + 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;
>> @@ -262,10 +248,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");
>>
>> ret = perf_session__process_events(session);
>>
>> @@ -273,11 +261,30 @@ static int report_raw_events(struct perf_mem *mem)
>> perf_session__delete(session);
>> return ret;
>> }
>> +static char *get_sort_order(struct perf_mem *mem)
>> +{
>> + char sort[MAX_SORT_ORDER_STR];
>> +
>> + /*
>> + * there is no weight (cost) associated with stores, so don't print
>> + * the column
>> + */
>> + if (mem->operation & MEM_OPERATION_STORE)
>> + strcpy(sort, "--sort=mem,sym,dso,symbol_daddr,dso_daddr,tlb,locked");
>> + else
>> + strcpy(sort, default_mem_sort_order);
>> +
>> + if (mem->phys_addr)
>> + strcat(sort, ",phys_daddr");
>> +
>> + return strdup(sort);
>> +}
>>
>> static int report_events(int argc, const char **argv, struct perf_mem *mem)
>> {
>> const char **rep_argv;
>> int ret, i = 0, j, rep_argc;
>> + char *new_sort_order;
>>
>> if (mem->dump_raw)
>> return report_raw_events(mem);
>> @@ -291,20 +298,9 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem)
>> rep_argv[i++] = "--mem-mode";
>> rep_argv[i++] = "-n"; /* display number of samples */
>>
>> - /*
>> - * there is no weight (cost) associated with stores, so don't print
>> - * the column
>> - */
>> - if (!(mem->operation & MEM_OPERATION_LOAD)) {
>> - if (mem->phys_addr)
>> - rep_argv[i++] = "--sort=mem,sym,dso,symbol_daddr,"
>> - "dso_daddr,tlb,locked,phys_daddr";
>> - else
>> - rep_argv[i++] = "--sort=mem,sym,dso,symbol_daddr,"
>> - "dso_daddr,tlb,locked";
>> - } else if (mem->phys_addr)
>> - rep_argv[i++] = "--sort=local_weight,mem,sym,dso,symbol_daddr,"
>> - "dso_daddr,snoop,tlb,locked,phys_daddr";
>> + new_sort_order = get_sort_order(mem);
>> + if (new_sort_order)
>> + rep_argv[i++] = new_sort_order;
>>
>> for (j = 1; j < argc; j++, i++)
>> rep_argv[i] = argv[j];
>> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
>> index ea8ff64..fc8290c 100644
>> --- a/tools/perf/util/sort.h
>> +++ b/tools/perf/util/sort.h
>> @@ -46,6 +46,8 @@ extern struct sort_entry sort_srcline;
>> extern enum sort_type sort__first_dimension;
>> extern const char default_mem_sort_order[];
>>
>> +#define MAX_SORT_ORDER_STR 128
>> +
>> struct he_stat {
>> u64 period;
>> u64 period_sys;
>> --
>> 2.7.4
>>
Powered by blists - more mailing lists