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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
