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

[Kanaka Juvva <kanaka.d.juvva@intel.com>]

On Thu, 2015-09-10 at 14:58 +0100, Matt Fleming wrote:
> 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.
> 
OK. QOS_EVENT_MASK can't be applied. Code changes will be done as per
this.
> > > > 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.
> 
> 

OK

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

>From my previous review:

  "This is completely backwards.
 
        tmp32 = rmid;  
        rmid = vrmid;
        do_stuff(rmid);
        rmid = tmp32;
        do_other_stuff(rmid);

   Why can't you use vrmid for do_stuff() and leave rmid alone? Just
   because it would make the code simpler to read?"

I have included Thomas comment inline above.

and also I meant the following logic:

writemsr(..,rmid,...)
mbm_*[vrmid]  

So new patch will use this logic.

> > > 
> > > 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.
>   
OK
> > > > > > @@ -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.
> 
> 
OK, WARN_ON I used for debugging.