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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 18 Apr 2016 16:03:03 +0000
From:	"Liang, Kan" <kan.liang@...el.com>
To:	"acme@...nel.org" <acme@...nel.org>, 'Jiri Olsa' <jolsa@...hat.com>
CC:	"'ak@...ux.intel.com'" <ak@...ux.intel.com>,
	"'alexander.shishkin@...ux.intel.com'" 
	<alexander.shishkin@...ux.intel.com>,
	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>
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, kan.liang@...el.com wrote:
> > > From: Kan Liang <kan.liang@...el.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@...el.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
> > >