From: "Liang, Kan" <kan.liang@intel.com>
To: Jiri Olsa <jolsa@redhat.com>
CC: "acme@kernel.org" <acme@kernel.org>,
        "namhyung@kernel.org" <namhyung@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH V6 1/3] perf tool: Add sort key symoff for perf diff
Thread-Topic: [PATCH V6 1/3] perf tool: Add sort key symoff for perf diff
Thread-Index: AQHQDXd2X8Mmnu1wKEOKtRcAXI3AlZx6n/yAgACGcOA=
Date: Mon, 1 Dec 2014 20:05:48 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07701677D8E@SHSMSX103.ccr.corp.intel.com>
References: <1417444812-22063-1-git-send-email-kan.liang@intel.com>
 <20141201195345.GA13302@krava.brq.redhat.com>
In-Reply-To: <20141201195345.GA13302@krava.brq.redhat.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org


> On Mon, Dec 01, 2014 at 09:40:10AM -0500, Kan Liang wrote:
> 
> SNIP
> 
> > +static int64_t
> > +sort__symoff_collapse(struct hist_entry *left, struct hist_entry
> > +*right) {
> > +	struct symbol *sym_l = left->ms.sym;
> > +	struct symbol *sym_r = right->ms.sym;
> > +	u64 symoff_l, symoff_r;
> > +	int64_t ret;
> > +
> > +	if (!sym_l || !sym_r)
> > +		return cmp_null(sym_l, sym_r);
> > +
> > +	ret = strcmp(sym_r->name, sym_l->name);
> > +	if (ret)
> > +		return ret;
> > +
> > +
> > +	symoff_l = left->ip - sym_l->start;
> > +	symoff_r = right->ip - sym_r->start;
> > +
> > +	return (int64_t)(symoff_r - symoff_l); }
> > +
> > +static int hist_entry__symoff_snprintf(struct hist_entry *he, char *bf,
> > +				    size_t size, unsigned int width) {
> > +	struct map *map = he->ms.map;
> > +	struct symbol *sym = he->ms.sym;
> > +	size_t ret = 0;
> > +
> > +	if (sym) {
> > +		ret += repsep_snprintf(bf + ret, size - ret, "%s", sym-
> >name);
> > +		ret += repsep_snprintf(bf + ret, size - ret, "+0x%llx",
> > +				       he->ip - sym->start);
> > +
> > +	} else {
> > +		size_t len = BITS_PER_LONG / 4;
> > +
> > +		ret += repsep_snprintf(bf + ret, size - ret, "%-#.*llx", len,
> > +				       map ? map->unmap_ip(map, he->ip) :
> he->ip);
> > +	}
> > +
> > +	ret += repsep_snprintf(bf + ret, size - ret, "%-*s",
> > +			       width - ret, "");
> > +	return ret;
> > +}
> > +
> > +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,
> > +};
> 
> I might have missed this in previous discussions, but do we also want just
> pure string comparison?
> 
> now I get:
>      0.30%   +0.42%  main+0x110
>      1.80%   -0.45%  main+0x115
>      0.05%   -0.04%  main+0x118
>      0.34%   +0.49%  main+0x11c
>      2.15%   -0.22%  main+0x120
>      0.41%   +0.22%  main+0x123
>      1.86%   +0.04%  main+0x12f
>      3.86%   -0.69%  main+0x133
>      0.02%           main+0x137
>      3.80%   -1.19%  main+0x13d
>      0.26%   +0.45%  main+0x141
>      2.26%   +1.41%  main+0x145
>      8.78%   -1.59%  main+0x148
>      0.05%           main+0x14c
>      1.40%   -0.26%  main+0x155
>      0.07%           main+0x158
>      0.74%   -0.03%  main+0x15b
>      1.06%   -0.17%  main+0x160
>      0.31%   +0.30%  main+0x1a8
>      1.82%   -0.51%  main+0x1af
>      0.09%   +0.07%  main+0x1b1
>      0.05%           main+0x1b4
> 
> could we add something like '-s symstr' to do only symbol string
> comparison, so the previous output would gather in single line like:
> 
>      5.09%   +2.07%  main

Yes, we have a patch to do that.
https://lkml.org/lkml/2014/11/21/367
The default sort key "symbol" do symbols names comparison.

The symoff here is an extension in case anyone want to do deeper analysis.

Thanks,
Kan

> 
> 
> otherwise the patchset looks ok to me:
>   Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> 
> thanks,
> jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/