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