2016-04-04 20:28:59

by Liang, Kan

[permalink] [raw]
Subject: [PATCH 1/1] perf tools: Fix format value calculation

From: Kan Liang <[email protected]>

The calculation of format value also rely on the continuity of the
format. However, uncore event format is not continuous.
E.g. The bit 21 as qpi event is lost.

perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00,
config2=0x3FE00/ -vvv
------------------------------------------------------------
perf_event_attr:
type 10
size 112
config 0x38



This patch checks the bit according to the bit position.

Signed-off-by: Kan Liang <[email protected]>
---
tools/perf/util/pmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index bf34468..47c096c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head *formats, const char *name)
static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
bool zero)
{
- unsigned long fbit, vbit;
+ unsigned long fbit;

- for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
+ for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {

if (!test_bit(fbit, format))
continue;

- if (value & (1llu << vbit++))
+ if (value & (1llu << fbit))
*v |= (1llu << fbit);
else if (zero)
*v &= ~(1llu << fbit);
--
2.5.5


2016-04-05 00:16:47

by Jiri Olsa

[permalink] [raw]
Subject: Re: [PATCH 1/1] perf tools: Fix format value calculation

On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:
> From: Kan Liang <[email protected]>
>
> The calculation of format value also rely on the continuity of the
> format. However, uncore event format is not continuous.
> E.g. The bit 21 as qpi event is lost.
>
> perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00,
> config2=0x3FE00/ -vvv
> ------------------------------------------------------------
> perf_event_attr:
> type 10
> size 112
> config 0x38

could you please share the event's format?

would be great to have some simple automated test for this one..

thanks,
jirka

>
>
>
> This patch checks the bit according to the bit position.
>
> Signed-off-by: Kan Liang <[email protected]>
> ---
> tools/perf/util/pmu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index bf34468..47c096c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head *formats, const char *name)
> static void pmu_format_value(unsigned long *format, __u64 value, __u64 *v,
> bool zero)
> {
> - unsigned long fbit, vbit;
> + unsigned long fbit;
>
> - for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
> + for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
>
> if (!test_bit(fbit, format))
> continue;
>
> - if (value & (1llu << vbit++))
> + if (value & (1llu << fbit))
> *v |= (1llu << fbit);
> else if (zero)
> *v &= ~(1llu << fbit);
> --
> 2.5.5
>

2016-04-05 03:14:26

by Liang, Kan

[permalink] [raw]
Subject: RE: [PATCH 1/1] perf tools: Fix format value calculation


> On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:
> > From: Kan Liang <[email protected]>
> >
> > The calculation of format value also rely on the continuity of the
> > format. However, uncore event format is not continuous.
> > E.g. The bit 21 as qpi event is lost.
> >
> > perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00,
> > config2=0x3FE00/ -vvv
> > ------------------------------------------------------------
> > perf_event_attr:
> > type 10
> > size 112
> > config 0x38
>
> could you please share the event's format?
>

cat /sys/devices/uncore_qpi_0/format/event
config:0-7,21

> would be great to have some simple automated test for this one..
>

It looks there is a test case in perf test.
7: Test perf pmu format parsing
But it looks there are some issues for the test case.

The test format with config is
{ "krava01", "config:0-1,62-63\n", },
{ "krava02", "config:10-17\n", },
{ "krava03", "config:5\n", },
The test input is
{
.config = (char *) "krava01",
.val.num = 15,
.type_val = PARSE_EVENTS__TERM_TYPE_NUM,
.type_term = PARSE_EVENTS__TERM_TYPE_USER,
},
{
.config = (char *) "krava02",
.val.num = 170,
.type_val = PARSE_EVENTS__TERM_TYPE_NUM,
.type_term = PARSE_EVENTS__TERM_TYPE_USER,
},
{
.config = (char *) "krava03",
.val.num = 1,
.type_val = PARSE_EVENTS__TERM_TYPE_NUM,
.type_term = PARSE_EVENTS__TERM_TYPE_USER,
},

The input value of "krava01" is 15 (0xf). The format of "krava01" is "config:0-1,62-63\n".
Apparently, the input has wrong format. But it looks the test case doesn't think so.
Also, at the end of the test case, it expects attr.config == 0xc00000000002a823.
I think it doesn't make sense either.

Any thoughts?

Thanks,
Kan

> thanks,
> jirka
>
> >
> >
> >
> > This patch checks the bit according to the bit position.
> >
> > Signed-off-by: Kan Liang <[email protected]>
> > ---
> > tools/perf/util/pmu.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index
> > bf34468..47c096c 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head
> > *formats, const char *name) static void pmu_format_value(unsigned long
> *format, __u64 value, __u64 *v,
> > bool zero)
> > {
> > - unsigned long fbit, vbit;
> > + unsigned long fbit;
> >
> > - for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
> > + for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
> >
> > if (!test_bit(fbit, format))
> > continue;
> >
> > - if (value & (1llu << vbit++))
> > + if (value & (1llu << fbit))
> > *v |= (1llu << fbit);
> > else if (zero)
> > *v &= ~(1llu << fbit);
> > --
> > 2.5.5
> >

2016-04-18 16:03:26

by Liang, Kan

[permalink] [raw]
Subject: RE: [PATCH 1/1] perf tools: Fix format value calculation

Hi all,

There is confusion about the usage of format for me.

E.g. The event config is not continuous for uncore.
cat /sys/devices/uncore_qpi_0/format/event
config:0-7,21

I once thought that the user input should be
uncore_qpi_0/event=0x200038,..../
So I submitted this patch and previous patch
"ac0e2cd555373ae6f8f3a3ad3fbbf5b6d1e7aaaa"

But I took a deep look of the code, it looks current implementation
expects the input as below.
uncore_qpi_0/event=0x138,..../

I didn't find any documents about this case.
Could you please confirm about it?
If 0x138 is the expected input, I think we need to make it clear in the
document.

Thanks,
Kan
>
>
> > On Mon, Apr 04, 2016 at 06:12:54AM -0700, [email protected] wrote:
> > > From: Kan Liang <[email protected]>
> > >
> > > The calculation of format value also rely on the continuity of the
> > > format. However, uncore event format is not continuous.
> > > E.g. The bit 21 as qpi event is lost.
> > >
> > > perf stat -a -e uncore_qpi_0/event=0x200038,config1=0x1C00,
> > > config2=0x3FE00/ -vvv
> > > ------------------------------------------------------------
> > > perf_event_attr:
> > > type 10
> > > size 112
> > > config 0x38
> >
> > could you please share the event's format?
> >
>
> cat /sys/devices/uncore_qpi_0/format/event
> config:0-7,21
>
> > would be great to have some simple automated test for this one..
> >
>
> It looks there is a test case in perf test.
> 7: Test perf pmu format parsing
> But it looks there are some issues for the test case.
>
> The test format with config is
> { "krava01", "config:0-1,62-63\n", },
> { "krava02", "config:10-17\n", },
> { "krava03", "config:5\n", },
> The test input is
> {
> .config = (char *) "krava01",
> .val.num = 15,
> .type_val = PARSE_EVENTS__TERM_TYPE_NUM,
> .type_term = PARSE_EVENTS__TERM_TYPE_USER,
> },
> {
> .config = (char *) "krava02",
> .val.num = 170,
> .type_val = PARSE_EVENTS__TERM_TYPE_NUM,
> .type_term = PARSE_EVENTS__TERM_TYPE_USER,
> },
> {
> .config = (char *) "krava03",
> .val.num = 1,
> .type_val = PARSE_EVENTS__TERM_TYPE_NUM,
> .type_term = PARSE_EVENTS__TERM_TYPE_USER,
> },
>
> The input value of "krava01" is 15 (0xf). The format of "krava01" is "config:0-
> 1,62-63\n".
> Apparently, the input has wrong format. But it looks the test case doesn't
> think so.
> Also, at the end of the test case, it expects attr.config ==
> 0xc00000000002a823.
> I think it doesn't make sense either.
>
> Any thoughts?
>
> Thanks,
> Kan
>
> > thanks,
> > jirka
> >
> > >
> > >
> > >
> > > This patch checks the bit according to the bit position.
> > >
> > > Signed-off-by: Kan Liang <[email protected]>
> > > ---
> > > tools/perf/util/pmu.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index
> > > bf34468..47c096c 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -586,14 +586,14 @@ __u64 perf_pmu__format_bits(struct list_head
> > > *formats, const char *name) static void pmu_format_value(unsigned
> > > long
> > *format, __u64 value, __u64 *v,
> > > bool zero)
> > > {
> > > - unsigned long fbit, vbit;
> > > + unsigned long fbit;
> > >
> > > - for (fbit = 0, vbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
> > > + for (fbit = 0; fbit < PERF_PMU_FORMAT_BITS; fbit++) {
> > >
> > > if (!test_bit(fbit, format))
> > > continue;
> > >
> > > - if (value & (1llu << vbit++))
> > > + if (value & (1llu << fbit))
> > > *v |= (1llu << fbit);
> > > else if (zero)
> > > *v &= ~(1llu << fbit);
> > > --
> > > 2.5.5
> > >