Skip to contentSkip to conversation

arm_mpam: Account for a MBWU overflow before reading the counter value

PATCH In review Ping Li 2 participants 2 messages Nov 11 MPAM driver

lore
Share & tools
lore

Ping Li

author
#0 lore raw

PATCH 1/1 arm_mpam: Account for a MBWU overflow before reading the counter value

While checking the driver's overflow handling and NRDY behaviour with the design engineering team, the following problem was found in __ris_msmon_read_locked():

__ris_msmon_read_locked() reads the MSMON_CFG_MBWU_CTL overflow status bit, clears it, then reads the counter value. The counter value read is where the overflow is accounted for, by adding the counter size to the per-monitor correction value.

If the monitor reports NRDY when the counter value is read, __ris_msmon_read_locked() returns -EBUSY, and mpam_msmon_read() retries after waiting. The overflow status bit was already cleared, so the retry reads it as 0, and the overflow is never accounted for. Each occurrence silently loses a counter-size increment from the accumulated bandwidth count.

The overflow status bit was read before it was cleared, so the overflow has already happened. Account for it before attempting to read the counter value, as the cleared status bit cannot be re-read.

Signed-off-by Ping Li <[email protected]>
---
 drivers/resctrl/mpam_devices.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/resctrl/mpam_devices.c +10 -3
index 27abf60f4d67..5384c9a4a1ad 100644
@@ -1528,6 +1528,16 @@ static void __ris_msmon_read_locked(struct mon_read *m)
 			return;
 	}
 
+	/*
+	 * The overflow status bit was read above, before being cleared by
+	 * software. The overflow has already happened, so account for it
+	 * even if reading the counter value below fails, as the cleared
+	 * status bit cannot be re-read.
+	 */
+	if (overflow)
+		ris->mbwu_state[ctx->mon].correction +=
+			mpam_msmon_overflow_val(m->type, msc);
+
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
 		m->err = mpam_read_monsel_reg(msc, CSU, &now32);

@@ -1569,9 +1579,6 @@ static void __ris_msmon_read_locked(struct mon_read *m)
 
 		mbwu_state = &ris->mbwu_state[ctx->mon];
 
-		if (overflow)
-			mbwu_state->correction += mpam_msmon_overflow_val(m->type, msc);
-
 		/*
 		 * Include bandwidth consumed before the last hardware reset and
 		 * a counter size increment for each overflow.
Signature · 1 line
2.34.1
maintainer
#1 lore raw

Hi Ping,

Quoted 9 lines · Ping Li

While checking the driver's overflow handling and NRDY behaviour with the design engineering team, the following problem was found in __ris_msmon_read_locked():

__ris_msmon_read_locked() reads the MSMON_CFG_MBWU_CTL overflow status bit, clears it, then reads the counter value. The counter value read is where the overflow is accounted for, by adding the counter size to the per-monitor correction value.

Quoted 6 lines

If the monitor reports NRDY when the counter value is read, __ris_msmon_read_locked() returns -EBUSY, and mpam_msmon_read() retries after waiting. The overflow status bit was already cleared, so the retry reads it as 0, and the overflow is never accounted for. Each occurrence silently loses a counter-size increment from the accumulated bandwidth count.

I was under the impression that this couldn't happen. Is the a theoretical fix or has this been observed in practice?

From IHI0099B.d 6.2.2 I_PCDHG regarding MBWU and the NRDY:
"
If 1, then the monitor instance is not ready and the VALUE field does not have an accurate count or measurement due to recent changes to the monitor settings.
"

Based on this, for MBWU you should only see NRDY set in the MAX_NRDY_USEC after a change of monitor settings. At such a reset the VALUE is reset to 0. Are you seeing or potentially seeing an overflow in this window of time?

Looking at 6.2.4 I_RZWDC we have "When an overflow occurs... The overflowed monitor VALUE and NRDY fields can be reset to zero" but the use of 'can' rather than 'are' renders that statement mostly useless.

Thanks,

Ben

Quoted 41 lines

The overflow status bit was read before it was cleared, so the overflow has already happened. Account for it before attempting to read the counter value, as the cleared status bit cannot be re-read.

Signed-off-by Ping Li <[email protected]>
---
 drivers/resctrl/mpam_devices.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/resctrl/mpam_devices.c +10 -3
index 27abf60f4d67..5384c9a4a1ad 100644
@@ -1528,6 +1528,16 @@ static void __ris_msmon_read_locked(struct mon_read *m)
 			return;
 	}
 
+	/*
+	 * The overflow status bit was read above, before being cleared by
+	 * software. The overflow has already happened, so account for it
+	 * even if reading the counter value below fails, as the cleared
+	 * status bit cannot be re-read.
+	 */
+	if (overflow)
+		ris->mbwu_state[ctx->mon].correction +=
+			mpam_msmon_overflow_val(m->type, msc);
+
 	switch (m->type) {
 	case mpam_feat_msmon_csu:
 		m->err = mpam_read_monsel_reg(msc, CSU, &now32);

@@ -1569,9 +1579,6 @@ static void __ris_msmon_read_locked(struct mon_read *m)
 
 		mbwu_state = &ris->mbwu_state[ctx->mon];
 
-		if (overflow)
-			mbwu_state->correction += mpam_msmon_overflow_val(m->type, msc);
-
 		/*
 		 * Include bandwidth consumed before the last hardware reset and
 		 * a counter size increment for each overflow.