Re: [PATCH v2 14/23] x86/resctrl: Calculate bandwidth from the previous __mon_event_count() chunks

From: Reinette Chatre
Date: Fri Oct 29 2021 - 18:22:36 EST

Hi James,

On 10/29/2021 8:50 AM, James Morse wrote:
On 27/10/2021 21:41, Reinette Chatre wrote:
On 10/27/2021 9:50 AM, James Morse wrote:
On 15/10/2021 23:28, Reinette Chatre wrote:
On 10/1/2021 9:02 AM, James Morse wrote:
mbm_bw_count() is only called by the mbm_handle_overflow() worker once a
second. It reads the hardware register, calculates the bandwidth and
updates m->prev_bw_msr which is used to hold the previous hardware register
value.

Operating directly on hardware register values makes it difficult to make
this code architecture independent, so that it can be moved to /fs/,
making the mba_sc feature something resctrl supports with no additional
support from the architecture.
Prior to calling mbm_bw_count(), mbm_update() reads from the same hardware
register using __mon_event_count().

Looking back I think 06c5fe9b12dd ("x86/resctrl: Fix incorrect local bandwidth when mba_sc
is enabled") may explain how the code ended up the way it is.

Change mbm_bw_count() to use the current chunks value from
__mon_event_count() to calculate bandwidth. This means it no longer
operates on hardware register values.

ok ... so could the patch just do this as it is stated here? The way it is implemented is
very complicated and hard (for me) to verify the correctness (more below).

diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
b/arch/x86/kernel/cpu/resctrl/monitor.c
index 6c8226987dd6..a1232462db14 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c

   static void mbm_bw_count(u32 rmid, struct rmid_read *rr)
   {
       struct rdt_hw_resource *hw_res = resctrl_to_arch_res(rr->r);
       struct mbm_state *m = &rr->d->mbm_local[rmid];
-    u64 tval, cur_bw, chunks;
+    u64 cur_bw, chunks, cur_chunks;
   -    tval = __rmid_read(rmid, rr->evtid);
-    if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
-        return;
+    cur_chunks = rr->val;
+    chunks = cur_chunks - m->prev_bw_chunks;
+    m->prev_bw_chunks = cur_chunks;
   -    chunks = mbm_overflow_count(m->prev_bw_msr, tval, hw_res->mbm_width);
-    cur_bw = (get_corrected_mbm_count(rmid, chunks) * hw_res->mon_scale) >> 20;
+    cur_bw = (chunks * hw_res->mon_scale) >> 20;

I find this quite confusing. What if a new m->prev_chunks is introduced instead and
initialized in __mon_event_count() to the value of chunks, and then here in mbm_bw_count
it could just retrieve it (chunks = m->prev_chunks).

(I agree the diff is noisy, it may be easier as a new function as this is a replacement
not a transform of the existing function)

__mon_event_count() can also be triggered by user-space reading the file, so any of its
'prev' values should be ignored, as they aren't updated on the 1-second timer needed to
get this in MB/s.

The resource group's mutex is taken before __mon_event_count() is called via user-space or
via the overflow handler so I think that mbm_bw_count() can assume that the prev values
are from the __mon_event_count() called just before it.

That is true. But changing this to work with the overflow+corrected value directly means
it doesn't need changing again as each of those steps are moved into the architecture
specific function. Changing this would make the later patches noisier, and we would have
the same discussion on a later patch.

ok



__mon_event_count()'s chunks values hasn't been through get_corrected_mbm_count(), so it
would need to be rr->val, which is what this code starts with for the "number of chunks
ever read by this counter".

The value could be corrected in mbm_bw_count(), no?

It could, but the aim of the series is to move all the architecture specific behaviour
behind an arch helper.

ok - I am still working on understanding how these helpers are organized


MPAMs counters read in bytes, and when they don't, its up to the MPAM architecture
specific code to fix the hardware values before resctrl gets them.

There is no reason for the mba_sc code to be architecture specific, it operates on the
counters and controls.


The variable 'chunks' has been used too much here, so its lost its meaning. How about:
|    current_chunk_count = rr->val;
|    delta_counter = current_chunk_count - m->prev_chunk_count;
|    cur_bw = (delta_counter * hw_res->mon_scale) >> 20;
|
|    m->prev_chunk_count = current_chunk_count;


The 'delta_counter' step was previously hidden in mbm_overflow_count(), which also had to
do with overflow of the hardware counter. Because a previously sanitised value is being
used, the hardware counters resolution doesn't need to be considered.
(which helps make mba_sc architecture independent)

This is the part that is not obvious to me: is the difference between the two individually
sanitized measurements the same as sanitizing the difference between the two measurements?

I agree get_corrected_mbm_count()'s rmid check and shift hide what it is doing, but it
boils down to a multiply. The existing code is (a - b)*cf, which is the same as this a*cf
- b*cf.

I'm not worried about this going wrong after 18-and-a-bit Exabytes of data is transferred,
at current memory speeds that would take decades. But: none of the 'cf' values are greater
than two, and the hardware register has two bits taken for error codes, so there is no a
or b that hardware can represent, with a cf less than two, that overflows a 64bit unsigned
long.

Thank you for answering it in this way. This seems fair. Could the commit message please elaborate more on the changes involved? The current summary of "Change mbm_bw_count() to use the current chunks value from __mon_event_count() to calculate bandwidth." is too cryptic (for me).

Reinette