Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key

[Arnaldo Carvalho de Melo]

Em Thu, Sep 22, 2016 at 05:36:48PM +0200, Jiri Olsa escreveu:
> Adding dcacheline dimension key support. It
> displays cacheline address as hex number.
> 
> Using c2c wrapper to standard 'dcacheline' object
> to defined own header and simple (just address)
> cacheline output.
> 
> Link: http://lkml.kernel.org/n/tip-j5enppr8e7h27nskqhgq33lu@xxxxxxxxxxxxxx
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
>  tools/perf/builtin-c2c.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index cfa12930b77b..335c0fd30757 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -1,5 +1,6 @@
>  #include <linux/compiler.h>
>  #include <linux/kernel.h>
> +#include <linux/stringify.h>
>  #include "util.h"
>  #include "debug.h"
>  #include "builtin.h"
> @@ -7,6 +8,7 @@
>  #include "mem-events.h"
>  #include "session.h"
>  #include "hist.h"
> +#include "sort.h"
>  #include "tool.h"
>  #include "data.h"
>  #include "sort.h"
> @@ -271,6 +273,33 @@ static int c2c_header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
>  	return scnprintf(hpp->buf, hpp->size, "%*s", width, text);
>  }
>  
> +static char *hex_str(u64 val)
> +{
> +	static char buf[20];

Ouch, what for?

> +
> +	snprintf(buf, 20, "0x%" PRIx64, val);
> +	return buf;
> +}
> +
> +static int64_t
> +dcacheline_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> +	       struct hist_entry *left, struct hist_entry *right)
> +{
> +	return sort__dcacheline_cmp(left, right);
> +}
> +
> +static int dcacheline_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
> +			    struct hist_entry *he)
> +{
> +	uint64_t addr = 0;
> +	int width = c2c_width(fmt, hpp, he->hists);
> +
> +	if (he->mem_info)
> +		addr = cl_address(he->mem_info->daddr.addr);
> +
> +	return snprintf(hpp->buf, hpp->size, "%*s", width, hex_str(addr));

So here you get that static buffer and then truncate it? Wouldn't the
perf_hpp stuff take care of this? Can't we stop using that static buffer
and this truncation at such a level?

> +}
> +
>  #define HEADER_LOW(__h)			\
>  	{				\
>  		.line[1] = {		\
> @@ -306,7 +335,16 @@ static int c2c_header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
>  		},			\
>  	}
>  
> +static struct c2c_dimension dim_dcacheline = {
> +	.header		= HEADER_LOW("Cacheline"),
> +	.name		= "dcacheline",
> +	.cmp		= dcacheline_cmp,
> +	.entry		= dcacheline_entry,
> +	.width		= 18,
> +};
> +
>  static struct c2c_dimension *dimensions[] = {
> +	&dim_dcacheline,
>  	NULL,
>  };
>  
> -- 
> 2.7.4