Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring (MBM) PMU

[Matt Fleming <matt.fleming@intel.com>]

On Tue, 2015-09-08 at 18:06 +0100, Juvva, Kanaka D wrote:
> 
> There are two aspects:
> 
>  1) Programming MSRs
>  2) EVENT_ATTR_STR(llc_local_bw, intel_cqm_llc_local_bw, "event=0x04");
> 
>  1 is used for programming MSRs
>  2 event attribute for perf
>   
> 
>  For MBM_LOCAL_EVENT HW ID is 0x3. We don't want to use 0x3 for EVENT ATTR. 
> 
> If we use 0x3 for event_attribute
> 
> We can't clearly distinguish whether is EVENT 01 & EVENT 02 or EVENT 03 alone.
>  For perf event attribute  it has to be 0x04. Because 0x01 and 0x02 are used for other two events

You cannot combine events like this, the perf events are not a bitmask
so having MBM_LOCAL_EVENT_ID as 0x3 is fine.

Just look at the Intel RAPL code, it does the same thing. 0x3 is a
perfectly valid perf event attr value and it does not mean "combine perf
event attr 0x1 and 0x2".

If you're concerned about QOS_EVENT_MASK, you can probably just delete
that and replace the code that uses it with a switch statement or
equivalent.

> > > Explained in the comment. If mbm_read is called within in 100ms for
> > > the same rmid, we don’t have to process the sample.
> > 
> > The key piece of information you're missing here is that skipping these small
> > deltas is an optimization, because we avoid performing costly operations for
> > what would likely be a very minor change in the MBM data, right?
> > 
> > 
> 
> Yes, You are correct.

OK, thanks. Please include that point in your comment.


> 
> > >         } else {
> > >                 mbm_current = &mbm_total[vrmid];
> > >                 eventid     = QOS_MBM_TOTAL_EVENT_ID;
> > >         }
> > >         rmid = tmp32;
> > 
> > Why did you assign rmid to vrmid if you reassign it before it was used?
> > 
> > 
> 
> For MSR writes we use rmid value and for mbm_* arrary we use vrmid which is actual
> index.

What I'm saying is that the assignment rmid = vrmid looks unnecessary in
this piece of code.

> > 
> > I suspect the document you're referring to above is only available under NDA,
> > which makes it unsuitable for mention in the kernel source since a large number
> > of people won't have access to it.
> > 
> > Just explain that the way the hardware is designed puts an upper limit on how
> > quickly the counter can overflow, which is once per second.
> > 
> >
> 
> OK. I'll change this to "as per hardware functionality"

I don't think that provides enough detail. Instead, how about "The
hardware architectures assure us that the counter will overflow at most
once a second", because that at least tells us where this assertion came
from.
  
> > > > > @@ -1023,6 +1437,17 @@ static void intel_cqm_event_stop(struct
> > > > perf_event *event, int mode)
> > > > >      } else {
> > > > >              WARN_ON_ONCE(!state->rmid);
> > > > >      }
> > > > > +
> > > > > +   if (pmu) {
> > > > > +           if (pmu->n_active >  0) {
> > > >
> > > > What's the purpose of this check? In the previous version there was
> > > > a WARN_ON(), which made sense. Did it trigger and you decided to "work"
> > > > around it?
> > > >
> > >
> > > We actually meant to check if there are active events
> > 
> > I don't follow this answer. Are you saying that the WARN_ON() doesn't make
> > sense here?
> > 
> > 
> > 
> I can add  WARN_ON. But this  will always hit if there are no events. 

OK, it sounds like the original WARN_ON() didn't make any sense. In
which case you don't need to re-add it.