Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1495809 > unrolled thread
| Started by | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| First post | 2016-10-05 13:10 +0200 |
| Last post | 2016-10-05 15:30 +0200 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-10-05 13:10 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Jiri Olsa <jolsa@redhat.com> - 2016-10-05 14:50 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Jiri Olsa <jolsa@redhat.com> - 2016-10-05 15:10 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-10-05 15:30 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Jiri Olsa <jolsa@redhat.com> - 2016-10-05 15:40 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Jiri Olsa <jolsa@redhat.com> - 2016-10-05 16:10 +0200
Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-10-05 15:30 +0200
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2016-10-05 13:10 +0200 |
| Subject | Re: [PATCH 20/57] perf c2c report: Add dcacheline dimension key |
| Message-ID | <soSff-bT-3@gated-at.bofh.it> |
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@kernel.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?
> +
> + 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
[toc] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-05 14:50 +0200 |
| Message-ID | <soTO1-11H-5@gated-at.bofh.it> |
| In reply to | #1495809 |
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@kernel.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
>
> > +
> > + 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..
thanks,
jirka
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-05 15:10 +0200 |
| Message-ID | <soU7n-1nW-3@gated-at.bofh.it> |
| In reply to | #1495854 |
On Wed, Oct 05, 2016 at 02:45:37PM +0200, Jiri Olsa wrote:
SNIP
> > > +
> > > + 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, so it's cut later on, but it allows only for left-side alignment
while we use the right-side one
if I leave it on perf_hpp to deal with it I end up with following output:
(check the Cacheline column)
# Total Rmt ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit
# Index Cacheline records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1
# ..... .................. ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ..
#
0 0x3d2e300 273 0.53% 44 22 22 40 40 0 0 0 22 233 107 78
1 0x3d001c0 68 0.51% 22 1 21 2 2 0 0 2 25 66 30 7
2 0x3d00200 165 0.48% 22 2 20 20 20 0 0 0 20 145 89 34
3 0x3d5ca80 22 0.41% 19 2 17 3 3 0 0 0 17 19 0 0
while current code does:
# Total Rmt ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit
# Index Cacheline records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1
# ..... .................. ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ..
#
0 0x3d2e300 273 0.53% 44 22 22 40 40 0 0 0 22 233 107 78
1 0x3d001c0 68 0.51% 22 1 21 2 2 0 0 2 25 66 30 7
2 0x3d00200 165 0.48% 22 2 20 20 20 0 0 0 20 145 89 34
3 0x3d5ca80 22 0.41% 19 2 17 3 3 0 0 0 17 19 0 0
I'll make the snprintf/scnprintf replacement
based on your acme/tmp.perf/c2c.2
thanks,
jirka
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2016-10-05 15:30 +0200 |
| Message-ID | <soUqK-1wC-13@gated-at.bofh.it> |
| In reply to | #1495860 |
Em Wed, Oct 05, 2016 at 03:09:29PM +0200, Jiri Olsa escreveu: > On Wed, Oct 05, 2016 at 02:45:37PM +0200, Jiri Olsa wrote: > > SNIP > > > > > + > > > > + 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, so it's cut later on, but it allows only for left-side alignment > while we use the right-side one > > if I leave it on perf_hpp to deal with it I end up with following output: > (check the Cacheline column) Which is not _that_ bad, I guess it gets like that because we expect kernel addresses as well (longer)? [root@jouet ~]# grep icmp_rcv /proc/kallsyms | cut -d' ' -f 1 | wc -c 17 [root@jouet ~]# echo -n .................. | wc -c 18 [root@jouet ~]# How to indicate to the hpp code that we want right alignment? Namhyung? > > # Total Rmt ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit > # Index Cacheline records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 > # ..... .................. ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... .. > # > 0 0x3d2e300 273 0.53% 44 22 22 40 40 0 0 0 22 233 107 78 > 1 0x3d001c0 68 0.51% 22 1 21 2 2 0 0 2 25 66 30 7 > 2 0x3d00200 165 0.48% 22 2 20 20 20 0 0 0 20 145 89 34 > 3 0x3d5ca80 22 0.41% 19 2 17 3 3 0 0 0 17 19 0 0 > > > while current code does: > > # Total Rmt ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit > # Index Cacheline records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 > # ..... .................. ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... .. > # > 0 0x3d2e300 273 0.53% 44 22 22 40 40 0 0 0 22 233 107 78 > 1 0x3d001c0 68 0.51% 22 1 21 2 2 0 0 2 25 66 30 7 > 2 0x3d00200 165 0.48% 22 2 20 20 20 0 0 0 20 145 89 34 > 3 0x3d5ca80 22 0.41% 19 2 17 3 3 0 0 0 17 19 0 0 > > > I'll make the snprintf/scnprintf replacement > based on your acme/tmp.perf/c2c.2 Thanks, > thanks, > jirka
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-05 15:40 +0200 |
| Message-ID | <soUAq-1Ai-7@gated-at.bofh.it> |
| In reply to | #1495873 |
On Wed, Oct 05, 2016 at 10:26:58AM -0300, Arnaldo Carvalho de Melo wrote: > Em Wed, Oct 05, 2016 at 03:09:29PM +0200, Jiri Olsa escreveu: > > On Wed, Oct 05, 2016 at 02:45:37PM +0200, Jiri Olsa wrote: > > > > SNIP > > > > > > > + > > > > > + 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, so it's cut later on, but it allows only for left-side alignment > > while we use the right-side one > > > > if I leave it on perf_hpp to deal with it I end up with following output: > > (check the Cacheline column) > > Which is not _that_ bad, I guess it gets like that because we expect > kernel addresses as well (longer)? exactly.. also it's the case for other columns where you have different number lengths more often.. the right side alignment is more readable in this case jirka
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-05 16:10 +0200 |
| Message-ID | <soV3s-209-7@gated-at.bofh.it> |
| In reply to | #1495873 |
On Wed, Oct 05, 2016 at 10:26:58AM -0300, Arnaldo Carvalho de Melo wrote: > Em Wed, Oct 05, 2016 at 03:09:29PM +0200, Jiri Olsa escreveu: > > On Wed, Oct 05, 2016 at 02:45:37PM +0200, Jiri Olsa wrote: > > > > SNIP > > > > > > > + > > > > > + 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, so it's cut later on, but it allows only for left-side alignment > > while we use the right-side one > > > > if I leave it on perf_hpp to deal with it I end up with following output: > > (check the Cacheline column) > > Which is not _that_ bad, I guess it gets like that because we expect > kernel addresses as well (longer)? > > [root@jouet ~]# grep icmp_rcv /proc/kallsyms | cut -d' ' -f 1 | wc -c > 17 > [root@jouet ~]# echo -n .................. | wc -c > 18 > [root@jouet ~]# > > How to indicate to the hpp code that we want right alignment? Namhyung? hum, the hist_entry__snprintf_alignment just adds missing spaces if there's any.. I think we should allow the upper layer to align based on its needs but maybe change hist_entry__snprintf_alignment to also force the width (like cut the overlap) in case it overflows jirka
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2016-10-05 15:30 +0200 |
| Message-ID | <soUqK-1wC-9@gated-at.bofh.it> |
| In reply to | #1495854 |
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@kernel.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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web