[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161005132307.GB30363@kernel.org>
Date: Wed, 5 Oct 2016 10:23:07 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Jiri Olsa <jolsa@...hat.com>
Cc: Jiri Olsa <jolsa@...nel.org>, lkml <linux-kernel@...r.kernel.org>,
Don Zickus <dzickus@...hat.com>, Joe Mario <jmario@...hat.com>,
Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Namhyung Kim <namhyung@...nel.org>,
David Ahern <dsahern@...il.com>,
Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key
Em Wed, Oct 05, 2016 at 02:45:37PM +0200, Jiri Olsa escreveu:
> On Wed, Oct 05, 2016 at 08:01:41AM -0300, Arnaldo Carvalho de Melo wrote:
> > 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@git.kernel.org
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---
> > > 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?
>
> it's being used later on for short time hex number string
> which is psased right away to another buffer ;-)
>
> having just one thread I didn't see any harm, esp when it
> saved some code lines
I have no problems with gotos, but hate static vars used to return
vals... :-)
> >
> > > +
> > > + 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?
>
> I think we need to cut it on this level, but I actualy might recall some
> change you did for perf_hpp to cut this on column width later on?
>
> I'll check on that..
Ok, would be great to have this sorted out in a better way.
Powered by blists - more mailing lists