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

[Matt Fleming <matt@codeblueprint.co.uk>]

On Thu, 10 Sep, at 02:18:49PM, Kanaka Juvva wrote:
> > > 
> > > > >         } 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.
 
OK, let's pull the code in and discuss this with some context,

	u64 val, currentmsr, currentbw, diff_time, cma, bytes, index;                                                                                                                      
        bool overflow = false, first = false;
        ktime_t cur_time;
        u32 tmp32 = rmid, eventid;
        struct sample *mbm_current;
        u32 vrmid = rmid_2_index(rmid);
 
        rmid = vrmid;			<--------- This looks wrong
        cur_time = ktime_get();
        if (read_mbm_local) {
                mbm_current = &mbm_local[vrmid];
                eventid     =  QOS_MBM_LOCAL_EVENT_ID_HW;
                wrmsr(MSR_IA32_QM_EVTSEL, QOS_MBM_LOCAL_EVENT_ID_HW, rmid);	<---- Unneccesary because..
        } else {
                mbm_current = &mbm_total[vrmid];
                eventid     = QOS_MBM_TOTAL_EVENT_ID;
        }
        rmid = tmp32;
        wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);	<----- ... you write here
 
So you don't actually use 'rmid' within that if/else block. You can
probably get away with deleting 'tmp32' now that you've refactored
things.

-- 
Matt Fleming, Intel Open Source Technology Center