Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring (MBM) PMU | From | Kanaka Juvva <> | Date | Thu, 10 Sep 2015 14:18:49 -0700 |
| |
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.
| |