lkml.org 
[lkml]   [2015]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring (MBM) PMU
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


\
 
 \ /
  Last update: 2015-09-11 23:41    [W:0.138 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site