From: "Liang, Kan" <kan.liang@intel.com>
To: "acme@kernel.org" <acme@kernel.org>, "'Jiri Olsa'" <jolsa@redhat.com>
CC: "'ak@linux.intel.com'" <ak@linux.intel.com>,
        "'alexander.shishkin@linux.intel.com'" 
	<alexander.shishkin@linux.intel.com>,
        "'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/1] perf tools: Fix format value calculation
Thread-Topic: [PATCH 1/1] perf tools: Fix format value calculation
Thread-Index: AQHRjrCktd2fl9rGrEiyiGPuyyDGuZ95/TmAgACvcfCAFT7xwA==
Date: Mon, 18 Apr 2016 16:03:03 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077058E1B63@SHSMSX103.ccr.corp.intel.com>
References: <1459775574-7595-1-git-send-email-kan.liang@intel.com>
 <20160405001611.GA9484@krava.local>
 <37D7C6CF3E00A74B8858931C1DB2F077058AB61C@SHSMSX103.ccr.corp.intel.com>
In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F077058AB61C@SHSMSX103.ccr.corp.intel.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3759
Lines: 131

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
> > >