Re: [PATCH V3 3/3] perf tool: Add sort key symoff for perf diff

From: Namhyung Kim
Date: Mon Nov 17 2014 - 23:13:14 EST

Next message: Guenter Roeck: "Re: [PATCH] regulator: Ensure unique regulator debugfs directory names"
Previous message: Dave Jones: "Re: 3.18-rc2 perf add_event_to_ctx warning"
In reply to: kan . liang: "[PATCH V3 3/3] perf tool: Add sort key symoff for perf diff"
Next in thread: kan . liang: "[PATCH V3 1/3] perf tool: Fix perf diff symble sort issue"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi kan,

On Mon, 17 Nov 2014 09:52:19 -0500, kan liang wrote:
> From: Kan Liang <kan.liang@xxxxxxxxx>
>
> Sometime, especially debugging scaling issue, the function level diff
> may be high granularity. The user may want to do deeper diff analysis
> for some cache or lock issue. The "symoff" key can let the user sort
> differential profile by ips. This feature should only work when the
> perf.data comes from same binary.

It'd be better to include output of symoff here IMHO.

>
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> ---
> tools/perf/Documentation/perf-diff.txt | 8 ++++++--
> tools/perf/builtin-diff.c | 2 +-
> tools/perf/util/hist.c | 5 +++--
> tools/perf/util/hist.h | 1 +
> tools/perf/util/sort.c | 29 +++++++++++++++++++++++++++++
> tools/perf/util/sort.h | 2 ++
> 6 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
> index e463caa..9e13911 100644
> --- a/tools/perf/Documentation/perf-diff.txt
> +++ b/tools/perf/Documentation/perf-diff.txt
> @@ -50,8 +50,12 @@ OPTIONS
>
> -s::
> --sort=::
> - Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline.
> - Please see description of --sort in the perf-report man page.
> + Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline, symoff.
> +
> + - symoff: exact symbol + offset address executed at the time of sample.
> + (for same binary compare)
> +
> + For other keys, please see description of --sort in the perf-report man page.
>
> -t::
> --field-separator=::
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 1ce425d..03a4001 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -744,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, parent, cpu, srcline, symoff, ..."
> " 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 "
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 6e88b9e..3d8a71b 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -67,13 +67,14 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
> symlen = h->ms.sym->namelen + 4;
> if (verbose)
> symlen += BITS_PER_LONG / 4 + 2 + 3;
> - hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> } else {
> symlen = unresolved_col_width + 4 + 2;
> - hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> hists__set_unres_dso_col_len(hists, HISTC_DSO);
> }
>
> + hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> + hists__new_col_len(hists, HISTC_SYMOFF, symlen);
> +
> len = thread__comm_len(h->thread);
> if (hists__new_col_len(hists, HISTC_COMM, len))
> hists__set_col_len(hists, HISTC_THREAD, len + 6);
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index d0ef9a1..874b203 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -24,6 +24,7 @@ enum hist_filter {
>
> enum hist_column {
> HISTC_SYMBOL,
> + HISTC_SYMOFF,
> HISTC_DSO,
> HISTC_THREAD,
> HISTC_COMM,
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 5b6c57d..370a799 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -280,6 +280,34 @@ struct sort_entry sort_sym = {
> .se_width_idx = HISTC_SYMBOL,
> };
>
> +static int64_t
> +sort__addr_cmp(struct hist_entry *left, struct hist_entry *right)
> +{
> + return _sort__addr_cmp(left->ip, right->ip);
> +}
> +
> +static int hist_entry__addr_snprintf(struct hist_entry *he, char *bf,
> + size_t size, unsigned int width)
> +{
> + struct map *map = he->ms.map;
> + u64 ip;
> +
> + if (map)
> + ip = map->unmap_ip(map, he->ip);
> + else
> + ip = he->ip;
> +
> + return _hist_entry__sym_snprintf(NULL, NULL, ip,
> + he->level, bf, size, width);
> +}
> +
> +struct sort_entry sort_symoff = {
> + .se_header = "Symbol+Offset",
> + .se_cmp = sort__addr_cmp,
> + .se_snprintf = hist_entry__addr_snprintf,
> + .se_width_idx = HISTC_SYMOFF,
> +};

I meant something like below (not tested):

sort__symoff_cmp(left, right)
{
return _sort__addr_cmp(left->ip, right->ip);
}

sort__symoff_collapse(left, right)
{
int64_t ret;
u64 symoff_l, symoff_r;

if (!left->ms.sym || right->ms.sym)
return cmp_null(...);

ret = strcmp(left->ms.sym, right->ms.sym);
if (ret)
return ret;

symoff_l = left->ip - left->ms.sym->start;
symoff_r = right->ip - right->ms.sym->start;

return (int64_t)(symoff_r - symoff_l);
}

hist_entry__symoff_snprintf(he, bf, size, width)
{
size_t ret;

if (he->ms.sym)
ret = repsep_snprintf(bf, size, "%s+0x%llx", he->ms.sym->name,
he->ip - he->ms.sym->start);
else
ret = repsep_snprintf(bf, size, "%-#*llx",
BITS_PER_LONG / 4 + 2, he->ip); // unmap?

ret += repsep_snprintf(bf + ret, size - ret, "%-*s",
width - ret, "");

if (ret > width)
bf[width] = '\0';

return width;
}

struct sort_entry sort_symoff = {
.se_header = "Symbol+Offset",
.se_cmp = sort__symoff_cmp,
.se_snprintf = hist_entry__symoff_snprintf,
.se_width_idx = HISTC_SYMOFF,

...

int sort_dimension__add()
{
...
else if (sd->entry == &sort_symoff) {
if (sort__mode == SORT_MODE__DIFF)
se->entry->se_collapse = sort__symoff_collapse;
}
...
}

Thanks,
Namhyung

> +
> /* --sort srcline */
>
> static int64_t
> @@ -1170,6 +1198,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_SYMOFF, "symoff", sort_symoff),
> DIM(SORT_PARENT, "parent", sort_parent),
> DIM(SORT_CPU, "cpu", sort_cpu),
> DIM(SORT_SRCLINE, "srcline", sort_srcline),
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index c03e4ff..ea0122f 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -38,6 +38,7 @@ 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_symoff;
> extern struct sort_entry sort_parent;
> extern struct sort_entry sort_dso_from;
> extern struct sort_entry sort_dso_to;
> @@ -161,6 +162,7 @@ enum sort_type {
> SORT_COMM,
> SORT_DSO,
> SORT_SYM,
> + SORT_SYMOFF,
> 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/

Next message: Guenter Roeck: "Re: [PATCH] regulator: Ensure unique regulator debugfs directory names"
Previous message: Dave Jones: "Re: 3.18-rc2 perf add_event_to_ctx warning"
In reply to: kan . liang: "[PATCH V3 3/3] perf tool: Add sort key symoff for perf diff"
Next in thread: kan . liang: "[PATCH V3 1/3] perf tool: Fix perf diff symble sort issue"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]