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

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

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

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 <kan.liang@intel.com>
---
 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

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

[Jiri Olsa]

On Mon, Apr 04, 2016 at 06:12:54AM -0700, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> 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 <kan.liang@intel.com>
> ---
>  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
> 

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

[Liang, Kan]

> On Mon, Apr 04, 2016 at 06:12:54AM -0700, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > 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 <kan.liang@intel.com>
> > ---
> >  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
> >

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

[Liang, Kan]

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, kan.liang@intel.com wrote:
> > > From: Kan Liang <kan.liang@intel.com>
> > >
> > > 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 <kan.liang@intel.com>
> > > ---
> > >  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
> > >