Re: [PATCH 1/1] perf tools: perf diff for different binaries
From: Namhyung Kim
Date: Mon Nov 10 2014 - 00:00:48 EST
Hi Kan,
On Thu, 6 Nov 2014 14:16:23 +0000, Kan Liang wrote:
> The diff code doesnât define event_op mmap2, so it fails to get the symbol.
Looks like a bug in perf diff code. (It's too easy to miss... :-/ )
> You are right, itâs ip address. The meaning of symbol for diff and report should
> be same.
Agreed.
>
> But I still think the author intends to compare the ip address. Low granularity is
> still useful for debugging the scaling issue. So we'd better keep both of them,
> and give ip address sorting a proper key name.
>
> "ip" means "IP address executed at the time of sample "
> "symbol" means "name of function executed at the time of sample"
I think the term 'IP address' is confusing since users might expect
network address.
>
> What about the attached patch?
>
> Thanks,
> Kan
>
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 25114c9..3063fbd 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -357,6 +357,7 @@ static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,
> static struct perf_tool tool = {
> .sample = diff__process_sample_event,
> .mmap = perf_event__process_mmap,
> + .mmap2 = perf_event__process_mmap2,
> .comm = perf_event__process_comm,
> .exit = perf_event__process_exit,
> .fork = perf_event__process_fork,
Please send it as a separate fix.
> @@ -743,7 +744,7 @@ static const struct option options[] = {
> OPT_STRING('S', "symbols", &symbol_conf.sym_list_str, "symbol[,symbol...]",
> "only consider these symbols"),
> OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
> - "sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline, ..."
> + "sort by key(s): pid, comm, dso, symbol, ip, parent, cpu, srcline, ..."
With this, you also need to update the documentation.
> " Please refer the man page for the complete list."),
> OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
> "separator for columns, no spaces will be added between "
> @@ -1164,6 +1165,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
> if (setup_sorting() < 0)
> usage_with_options(diff_usage, options);
>
> + if (sort__has_ip)
> + tool.mmap2 = NULL;
> +
I don't think this is the right thing to do. And I guess you need to
identify symbols anyway.
> setup_pager();
>
> sort__setup_elide(NULL);
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 9402885..a59f389 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -20,6 +20,7 @@ int have_ignore_callees = 0;
> int sort__need_collapse = 0;
> int sort__has_parent = 0;
> int sort__has_sym = 0;
> +int sort__has_ip = 0;
Why is this needed?
> int sort__has_dso = 0;
> enum sort_mode sort__mode = SORT_MODE__NORMAL;
>
> @@ -272,9 +273,32 @@ static int hist_entry__sym_snprintf(struct hist_entry *he, char *bf,
> he->level, bf, size, width);
> }
>
> +static int64_t
> +sort__sym_name_cmp(struct hist_entry *left, struct hist_entry *right)
> +{
> + const char *sym_name_l, *sym_name_r;
> +
> + if (!left->ms.sym || !right->ms.sym)
> + return cmp_null(right->ms.sym, left->ms.sym);
> +
> + sym_name_l = left->ms.sym->name;
> + sym_name_r = right->ms.sym->name;
> +
> + return strcmp(sym_name_l, sym_name_r);
> +}
Looks like doing same thing as sort__sym_sort().
> +
> struct sort_entry sort_sym = {
> .se_header = "Symbol",
> .se_cmp = sort__sym_cmp,
> + .se_collapse = sort__sym_name_cmp,
This will change the behavior of perf report somewhat.
> + .se_sort = sort__sym_sort,
> + .se_snprintf = hist_entry__sym_snprintf,
> + .se_width_idx = HISTC_SYMBOL,
> +};
> +
> +struct sort_entry sort_ip = {
> + .se_header = "IP address",
> + .se_cmp = sort__sym_cmp,
I think what you need is "symbol+offset" comparison so the .se_cmp
should compare hist_entry->ip (ie. mapped addr) instead of sym->start.
But I'm still not sure how it's meaningful since a bit of change will
result in changing offsets so that it cannot find a match.
Thanks,
Namhyung
> .se_sort = sort__sym_sort,
> .se_snprintf = hist_entry__sym_snprintf,
> .se_width_idx = HISTC_SYMBOL,
> @@ -1170,6 +1194,7 @@ static struct sort_dimension common_sort_dimensions[] = {
> DIM(SORT_COMM, "comm", sort_comm),
> DIM(SORT_DSO, "dso", sort_dso),
> DIM(SORT_SYM, "symbol", sort_sym),
> + DIM(SORT_IP, "ip", sort_ip),
> DIM(SORT_PARENT, "parent", sort_parent),
> DIM(SORT_CPU, "cpu", sort_cpu),
> DIM(SORT_SRCLINE, "srcline", sort_srcline),
> @@ -1430,6 +1455,8 @@ int sort_dimension__add(const char *tok)
> sort__has_parent = 1;
> } else if (sd->entry == &sort_sym) {
> sort__has_sym = 1;
> + } else if (sd->entry == &sort_ip) {
> + sort__has_ip = 1;
> } else if (sd->entry == &sort_dso) {
> sort__has_dso = 1;
> }
> @@ -1809,6 +1836,7 @@ void reset_output_field(void)
> sort__need_collapse = 0;
> sort__has_parent = 0;
> sort__has_sym = 0;
> + sort__has_ip = 0;
> sort__has_dso = 0;
>
> field_order = NULL;
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index c03e4ff..f00900a 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -34,10 +34,12 @@ extern int have_ignore_callees;
> extern int sort__need_collapse;
> extern int sort__has_parent;
> extern int sort__has_sym;
> +extern int sort__has_ip;
> extern enum sort_mode sort__mode;
> extern struct sort_entry sort_comm;
> extern struct sort_entry sort_dso;
> extern struct sort_entry sort_sym;
> +extern struct sort_entry sort_ip;
> extern struct sort_entry sort_parent;
> extern struct sort_entry sort_dso_from;
> extern struct sort_entry sort_dso_to;
> @@ -161,6 +163,7 @@ enum sort_type {
> SORT_COMM,
> SORT_DSO,
> SORT_SYM,
> + SORT_IP,
> SORT_PARENT,
> SORT_CPU,
> SORT_SRCLINE,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/