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

From: Liang, Kan
Date: Mon Apr 18 2016 - 12:03:29 EST


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