Re: [PATCH v2 2/4] perf/x86: Fix data source decoding for Skylake

[Andi Kleen <andi@firstfloor.org>]

On Thu, Jun 08, 2017 at 12:40:59PM -0700, Stephane Eranian wrote:
> Hi,
> 
> On Thu, Jun 8, 2017 at 1:15 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Jun 07, 2017 at 04:22:24PM -0700, Andi Kleen wrote:
> >
> > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > > index b1c0b187acfe..95daade294d7 100644
> > > --- a/include/uapi/linux/perf_event.h
> > > +++ b/include/uapi/linux/perf_event.h
> > > @@ -931,14 +931,18 @@ union perf_mem_data_src {
> > >                       mem_snoop:5,    /* snoop mode */
> > >                       mem_lock:2,     /* lock instr */
> > >                       mem_dtlb:7,     /* tlb access */
> > > -                     mem_rsvd:31;
> > > +                     mem_lvlx:8,     /* memory hierarchy level, ext */
> > > +                     mem_snoopx:2,   /* snoop mode, ext */
> > > +                     mem_rsvd:21;
> > >       };
> > >  };
> > >  #elif defined(__BIG_ENDIAN_BITFIELD)
> > >  union perf_mem_data_src {
> > >       __u64 val;
> > >       struct {
> > > -             __u64   mem_rsvd:31,
> > > +             __u64   mem_rsvd:21,
> > > +                     mem_snoopx:2,   /* snoop mode, ext */
> > > +                     mem_lvlx:8,     /* memory hierarchy level, ext */
> > >                       mem_dtlb:7,     /* tlb access */
> > >                       mem_lock:2,     /* lock instr */
> > >                       mem_snoop:5,    /* snoop mode */
> >
> > So one thing we could do is add a mem_hops field and always set that,
> > even for the old stuff. The old stuff will not know about that field and
> > ignore the bits, but new stuff will then not need as many LVL bits.

Note that Skylake cannot fill it in, it doesn't report the hops.
And for the old parts the existing bits work. So I don't think
there's a motivation to add a new hops field, unless some other
architecture needs it.

> >
> That would be better than lvlx I think. I am guessing you're suggesting
> an integer count here and not a bitmask. Right? Then I wonder why it
> would need 8 bits or 255 possible levels!

Sure counts are better than bits. At least it's a far more efficient
encoding. Ok so add a new level_num field, and use one count for 
PMEM and L4, and also fill in for the others.

> > > +#define PERF_MEM_LVLX_PMEM   0x08 /* Persistent Memory */
> > > +#define PERF_MEM_LVLX_REM_PMEM       0x10 /* Remote Persistent Memory */
> > > +#define PERF_MEM_LVLX_REM_NA 0x20 /* Remote N/A level */
> >
> > Still wondering what the point of REM_NA is.. can you explain?

It's a remote cache, but the hardware doesn't report which one, 
and also doesn't report the hops.

Ok PERF_MEM_LVL_REM_CCE would be a better name I guess, fitting
with the existing ones.

-Andi