Messages in this thread Patch in this message | | | From | "Juvva, Kanaka D" <> | Subject | RE: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring (MBM) PMU | Date | Tue, 8 Sep 2015 16:11:45 +0000 |
| |
Hi Matt,
This is regarding Event IDs for perf event:
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 tow
Thanks, -Kanaka
> -----Original Message----- > From: Fleming, Matt > Sent: Tuesday, September 8, 2015 4:47 AM > To: Juvva, Kanaka D > Cc: Thomas Gleixner; Kanaka Juvva; Williamson, Glenn P; Auld, Will; Andi Kleen; > LKML; Luck, Tony; Peter Zijlstra; Tejun Heo; x86@kernel.org; Ingo Molnar; H. > Peter Anvin; Shivappa, Vikas > Subject: Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth Monitoring > (MBM) PMU > > On Mon, 2015-09-07 at 20:22 +0100, Juvva, Kanaka D wrote: > > Hi Thomas, > > > > I'm sending updated patch(s). I have given details for each of > > these items below. > > > > Kanaka, this email is HTML formatted and so has been blocked by > vger.kernel.org where the linux-kernel mailing list is hosted. > > Please configure outlook not to send html email, or use a different mail agent > for working with upstream. > > > Regards, > > -Kanaka > > > > > -----Original Message----- > > > From: Thomas Gleixner [mailto:tglx@linutronix.de] > > > Sent: Wednesday, August 19, 2015 1:50 PM > > > To: Kanaka Juvva > > > Cc: Juvva, Kanaka D; Williamson, Glenn P; Fleming, Matt; Auld, Will; > > Andi Kleen; > > > LKML; Luck, Tony; Peter Zijlstra; Tejun Heo; x86@kernel.org; Ingo > > Molnar; H. > > > Peter Anvin; Shivappa, Vikas > > > Subject: Re: [PATCH v3 1/2] perf,x86: add Intel Memory Bandwidth > > Monitoring > > > (MBM) PMU > > > > > > On Fri, 7 Aug 2015, Kanaka Juvva wrote: > > > > +#define MBM_CNTR_MAX 0xffffff > > > > +#define MBM_SOCKET_MAX 8 > > > > +#define MBM_TIME_DELTA_MAX 1000 > > > > +#define MBM_TIME_DELTA_MIN 100 > > > > > > What are these constants for and how are they determined? Pulled out > > > of thin air? > > > > > > > /* > > * MBM Counter is 24bits wide. MBM_CNTR_MAX defines max counter > > * value > > */ > > #define MBM_CNTR_MAX 0xffffff > > /* > > * Max #sockets supported > > */ > > #define MBM_SOCKET_MAX 8 > > This seems like a constant we could get by without. Do we really need to know > this at compile time? > > > /* > > * Expected time interval between consecutive MSR reads for a given rmid > > */ > > #define MBM_TIME_DELTA_MAX 1000 > > "max" and "expected" are not the same thing. > > > > > > #define QOS_L3_OCCUP_EVENT_ID (1 << 0) > > > > +#define QOS_MBM_TOTAL_EVENT_ID (1 << 1) > > > > +#define QOS_MBM_LOCAL_EVENT_ID_HW 0x3 > > > > +#define QOS_MBM_LOCAL_EVENT_ID (1 << 2) > > > > > > So we have ID values which are built with (1 << X) and then this HW > > > variant in the middle with 0x3. Of course without any explanation what the > heck this stuff is. > > > > > > Last review: > > > > > > "So this wants a descriptive ID name and a comment." > > > > > > > > > > /* > > * MBM Event IDs as defined in SDM section 17.14.6 > > * Event IDs used to program MSRs for reading counters > > */ > > #define QOS_MBM_TOTAL_EVENT_ID (1 << 1) > > #define QOS_MBM_LOCAL_EVENT_ID_HW 0x3 > > /* > > * Perf needs event id to be 1 << x, hence we can't use 0x3 (HW EVENT ID) > > * for MBM_LOCAL_EVENT we use next 1 << x for MBM_LOCAL_EVENT_ID > > */ > > #define QOS_MBM_LOCAL_EVENT_ID (1 << 2) > > No, perf events do not need to be of the form (1 << X), that was just a > convention we used in the cqm code before we knew what values the MBM > events would take - you can change these to be whatever format you want, but > be sure to make it consistent. > > The constants are very much supposed to be programmed into the MSRs, take a > look at __rmid_read(). > > I would suggest (as I already did privately) that you change the format to be > 0x0x for all of these event IDs. > > > > > > @@ static bool intel_cqm_sched_in_event(u32 rmid) > > > > return false; > > > > } > > > > > > > > + > > > > +static u32 bw_sum_calc(struct sample *bw_stat, int rmid) { > > > > + u32 val = 0, i, j, index; > > > > + > > > > + if (++bw_stat->fifoout >= mbm_window_size) > > > > + bw_stat->fifoout = 0; > > > > + index = bw_stat->fifoout; > > > > + for (i = 0; i < mbm_window_size - 1; i++) { > > > > + if (index + i >= mbm_window_size) > > > > + j = index + i - mbm_window_size; > > > > + else > > > > + j = index + i; > > > > + val += bw_stat->mbmfifo[j]; > > > > + } > > > > > > This math wants a explanatory comment. > > > > > /* > > * Slide the window by 1 and calculate the sum of the last > > * mbm_window_size-1 bandwidth values. > > * fifoout is the current position of the window. > > * Increment the fifoout by 1 to slide the window by 1. > > * > > * Calcalute the bandwidth from ++fifiout to ( ++fifoout + mbm_window_size - > 1) > > * e.g.fifoout =1; Bandwidth1 Bandwidth2 ..... Bandwidthn are the > > * sliding window values where n is size of the sliding window > > * bandwidth sum: val = Bandwidth2 + Bandwidth3 + .. Bandwidthn > > */ > > Instead of these large comment blocks please comment smaller, logically- > connected chunks of code, e.g. > > /* Slide the window by one */ > if (++bw_stat->fifoout >= mbm_window_size) > bw_stat->fifoout = 0; > > /* > * Calculate the sum of last mbm_window_size-1 values. > */ > for (i = 0; i < mbm_window_size - 1; i++) { > /* Handle wraparound at end of window */ > if (index + i >= mbm_window_size) > j = index + i - mbm_window_size; > else > j = index + i; > > val += bw_stat->mbminfo[j]; > } > > > > > > > > > + return val; > > > > +} > > > > + > > > > +static u32 __mbm_fifo_sum_lastn_out(int rmid, bool is_localbw) { > > > > + if (is_localbw) > > > > + return bw_sum_calc(&mbm_local[rmid], rmid); > > > > + else > > > > + return bw_sum_calc(&mbm_total[rmid], rmid); } > > > > + > > > > +static void __mbm_fifo_in(struct sample *bw_stat, u32 val) { > > > > + bw_stat->mbmfifo[bw_stat->fifoin] = val; > > > > + if (++bw_stat->fifoin >= mbm_window_size) > > > > > > How does that become greater than mbm_windowsize? > > > > > > > This is fixed by changing >= to == > > Added a comment: > > > > /* > > * store current sample's bw value in sliding window at the > > * index fifoin. Increment fifoin. Check if fifoin has reached > > * max_window_size. If yes reset it to begining i.e. zero > > * e.g. > > * mbm_window_size = 10 > > * mbmfifo is a circular fifo 0 1 2 3 4 5 6 7 8 9 10 > > * ^ | > > * | | > > * | _ _ _ _ _ _ _ _ _ _| > > * > > * So when fifoin becomes 10, then it is reset to zero > > * > > */ > > I'm not sure that this comment adds anything of value that isn't already > understood by reading the code. I don't think you need a comment for this > function, it seems pretty straight forward and Thomas' question was about the > boundary limits of ->fifoin. > > > > > + bw_stat->fifoin = 0; > > > > +} > > > > > > > +/* > > > > + * __rmid_read_mbm checks whether it is LOCAL or GLOBAL MBM event > > > > +and reads > > > > + * its MSR counter. Check whether overflow occurred and handles it. > > > > +Calculates > > > > + * currenet BW and updates running average. > > > > > > currenet? And please get rid of the double spaces > > > > > This is fixed now. Here is the updated comment: > > > > /* > > * rmid_read_mbm checks whether it is LOCAL or Total MBM event and reads > > * its MSR counter. Check whether overflow occured and handles it. Calculates > > * currenet BW and updates running average. > > * > > ^^^ You've still misspelled current. > > > > * Overflow Handling: > > * if (MSR current value < MSR previous value) it is an > > * overflow. MSR values are increasing when bandwidth consumption for the > thread > > * is non-zero; When MSR values reaches MAX_COUNTER_VALUE it overflows. > After overflow, > > * MSR current value goes back to zero and starts increasing again at the rate > of > > * bandwidth. > > * > > You don't need to provide a definition of "overflow", most people will be > familiar with it. What is more important to document is how the overflow is > handled... > > > > * Overflow handling: > > * Detect an overflow : current read value > last read value > > Isn't this inverted? Overflow occurred if current < previous. > > > > > * Overflow correction: if (overflow) > > * Current value = (MAX_COUNTER_VALUE - prev read value) + > current read value > > * else > > * Current value = current read value > > * > > Please don't write pseudocode in the comments. Use English prose to describe > the important parts of the code. > > > > > * Calculation of Current Bandwidth value: > > * If MSR is read within last 100ms, then then the smaple is ignored; > > * If the MSR was Read with in last 100ms, why incur an extra overhead > > * of doing the MSR reads again. Anyway there'll be a negligible change or zero > > * change in MSR readings in 100ms. > > * > > * Bandwidth is calculated as: > > * memory bandwidth = difference of last two msr counter values/time > difference. > > * > > * cum_avg = Running Average bandwidth of last 'n' bandwidth values for > > * the samples that are processed > > * > > Where 'n' is 'mbm_window_size' ? If so, please use 'mbm_window_size', not 'n'. > > > > * Sliding window is used to save the last 'n' samples. Where, > > * n = sliding_window_size and results in sliding window duration of 'n' secs. > > Hmm... this confuses me a lot. Is 'n' a size or a duration? The two are not the > same thing. > > > > > * The sliding window size by default set to > > * MBM_FIFO_SIZE_MIN. User can configure it to the values in the range > > * (MBM_FIFO_SIZE_MIN,MBM_FIFO_SIZE_MAX). The range for sliding window > > * is chosen based on a general criteria for monitoring duration. Example > > * for a short lived application, 10sec monitoring period gives > > * good characterization of its bandwidth consumption. For an application > > * that runs for longer duration, 300sec monitoring period gives better > > * characterization of its bandwidth consumption. Since the running average > > * calculated for total monitoring period, user gets the most accuracate > > * average bandwidth for the each monitoring period. > > * > > * Scaling: > > * cum_avg is the raw bandwidth is Bytes/sec. > > * cum_avg is converted to MB/sec by applying MBM_CONVERSION_FACTOR > and > > * rounded to nearest integer. User interface gets the Bandwidth values in > MB/sec. > > * > > */ > > > > + * > > > > + * Overflow Handling: > > > > + * if (MSR current value < MSR previous value) it is an > > > > + * overflow. and overflow is handled. > > > > > > Wow. That's informative as hell! > > > > > Please look at the modified comment above > > > > + * > > > > + * Calculation of Current BW value: > > > > > > BW == Body Weight? > > > > > > > It is fixed now > > > > > > + * If MSR is read within last 100ms, then the value is ignored; > > > > + * this will suppress small deltas. We don't process MBM samples > > > > + that are > > > > + * within 100ms. > > > > > > WHY? > > > > > 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? > > > > > > +{ > > > > + u64 val, tmp, diff_time, cma, bytes, index; > > > > + bool overflow = false, first = false; > > > > + ktime_t cur_time; > > > > + u32 tmp32 = rmid; > > > > + struct sample *mbm_current; > > > > + u32 vrmid = topology_physical_package_id(smp_processor_id()) * > > > > + cqm_max_rmid + rmid; > > > > + > > > > + rmid = vrmid; > > > > > > 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?" > > > > > > Still applies. > > > > > > > This is now changed to > > 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; > > 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); > > You don't need to perform this wrmsr() here because it's taken care of in the > common code below. > > > } 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? > > > > > > + /* if current msr value < previous msr value , it means overflow */ > > > > + if (val < bytes) { > > > > + val = MBM_CNTR_MAX - bytes + val; > > > > + overflow = true; > > > > + } else > > > > + val = val - bytes; > > > > + > > > > + val = (val * MBM_TIME_DELTA_MAX) / diff_time; > > > > + > > > > + if ((diff_time > MBM_TIME_DELTA_MAX) && (!cma)) > > > > + /* First sample */ > > > > + first = true; > > > > + > > > > + rmid = vrmid; > > > > > > And another time: > > > > > > "More obfuscation" > > > > > > > /* > > * MBM_TIME_DELTA_MAX is picked as per MBM specs. As specified in > Intel Platform > > * Quality of Service Monitoring Implementer's Guide V1, Section 2.7.2. > page 21, > > * overflow can occur maximum once in a second. So latest we want to > read the MSR > > * counters is 1000ms. If it is less than 1000ms we can ignore the sample. > Then we > > * decide since when we should ignore. If the MSR was Read with in last > 100ms, why > > * process the MSR reads again. Anyway there'll be small change or zero > change. > > * So ignoring MSR Reads within 100ms or less is efficient. > MBM_TIME_DELTA_MIN > > * is specified as 100ms as per this guideline. > > * > > */ > > 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. > > > > > > +static void __intel_cqm_event_total_bw_count(void *info) { > > > > + struct rmid_read *rr = info; > > > > + u64 val; > > > > + > > > > + val = __rmid_read_mbm(rr->rmid, false); > > > > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL)) > > > > + return; > > > > + atomic64_add(val, &rr->value); } > > > > + > > > > +static void __intel_cqm_event_local_bw_count(void *info) { > > > > + struct rmid_read *rr = info; > > > > + u64 val; > > > > + > > > > + val = __rmid_read_mbm(rr->rmid, true); > > > > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL)) > > > > + return; > > > > + atomic64_add(val, &rr->value); } > > > > > > And once more: > > > > > > "You're really a fan of copy and paste." > > > > > > > These functions are invoked indirectly. They were written keeping > intel_cqm_event_count in mind. > > I’ll change the arg to struct mbm_read{ > > struct rmid_read *rr; > > u32 eventid; > > }; > > Intel_cqm_event_*_bw_count(….) needs eventid to call for decoding > > No, please do not duplicate the rmid_read structure, that is not an > improvement, we don't need two different structs for reading the read data. > > Please add the event field to the existing struct rmid_read. > > > > > > @@ -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? > > > > > > > > > +EVENT_ATTR_STR(llc_total_bw.unit, intel_cqm_llc_total_bw_unit, > > > > +"KB/sec"); EVENT_ATTR_STR(llc_local_bw.unit, > > > > +intel_cqm_llc_local_bw_unit, "KB/sec"); #endif > > > > > > > +static ssize_t > > > > +sliding_window_size_store(struct device *dev, > > > > + struct device_attribute *attr, > > > > + const char *buf, size_t count) { > > > > + unsigned int bytes; > > > > + int ret; > > > > + > > > > + ret = kstrtouint(buf, 0, &bytes); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + mutex_lock(&cache_mutex); > > > > + if (bytes > 0 && bytes <= MBM_FIFO_SIZE_MAX) > > > > + mbm_window_size = bytes; > > > > > > So, it's valid to set the window to X where 0 < X < MBM_FIFO_SIZE_MIN. > > > What's the actual purpose of MBM_FIFO_SIZE_MIN? > > > > > This is changed to > > if (bytes >= MBM_FIFO_SIZE_MIN && bytes <= MBM_FIFO_SIZE_MAX) > > mbm_window_size = bytes; > > Note that if the user passes a value outside of this range you should be returning > -EINVAL to indicate that. > > > > > > + pmu->timer_interval = ms_to_ktime(MBM_TIME_DELTA_MAX); > > > > + per_cpu(mbm_pmu, cpu) = pmu; > > > > + per_cpu(mbm_pmu_to_free, cpu) = NULL; > > > > > > What's the point of this? If there is still something to be free'd its leaked. > > > Otherwise that's redundant. > > per_cpu(mbm_pmu_to_free, cpu) = NULL; is removed > > > > > > + mbm_hrtimer_init(pmu); > > > > + } > > > > + return 0; > > > > > > s/0/NOTIFY_OK/ because you return that value directly. > > > > > You mean I return the ‘return code’ > > ? > > You should be using NOTIFY_OK here so that you follow the notifier API > convention. >
| |