Add a counter and debug printf to change the latency inside tlb_sync

PATCH New Chao-ying Fu 3 participants 8 messages Jan 10, 2025

lore

Share & tools

lore

Chao-ying Fu

author

2025-01-10 00:03 UTC #0 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

The tlb_sync loop is slowed down by variable latency to avoid livelock on some platforms.

---
 lib/sbi/sbi_tlb.c | 6 ++++++
 1 file changed, 6 insertions(+)

lib/sbi/sbi_tlb.c +6 -0

index 01b31f4..d46b95c 100644
@@ -247,6 +247,7 @@ static void tlb_process(struct sbi_scratch *scratch)
 
 static void tlb_sync(struct sbi_scratch *scratch)
 {
+	int count = 0;
 	atomic_t *tlb_sync =
 			sbi_scratch_offset_ptr(scratch, tlb_sync_off);

@@ -256,6 +257,11 @@ static void tlb_sync(struct sbi_scratch *scratch)
 		 * consume fifo requests to avoid deadlock.
 		 */
 		tlb_process_once(scratch);
+		count++;
+		if ((count % 20) == 0) {
+			sbi_dprintf("tlb_sync loop count = %d value = %ld\n",
+				    count, atomic_read(tlb_sync));
+		}
 	}
 
 	return;

Signature · 1 line

2.47.1

Jessica Clarke

2025-01-10 00:52 UTC #1 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Quoted 3 lines · Chao-ying Fu

The tlb_sync loop is slowed down by variable latency to avoid livelock on some platforms.

I don?t understand ?by variable latency?. It doesn?t do anything but print.

Quoted 17 lines

---
lib/sbi/sbi_tlb.c | 6 ++++++
1 file changed, 6 insertions(+)
lib/sbi/sbi_tlb.c +1 -0
index 01b31f4..d46b95c 100644
@@ -247,6 +247,7 @@ static void tlb_process(struct sbi_scratch *scratch)
 
 static void tlb_sync(struct sbi_scratch *scratch)
 {
+ int count = 0;
atomic_t *tlb_sync =
sbi_scratch_offset_ptr(scratch, tlb_sync_off);

lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 260

Quoted 4 lines

lib/sbi/sbi_tlb.c +1 -0

@@ -256,6 +257,11 @@ static void tlb_sync(struct sbi_scratch *scratch)
 * consume fifo requests to avoid deadlock.
 */
 tlb_process_once(scratch);
+ count++;

Technically this could overflow, and count is signed, so UB...

lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 263

Quoted 3 lines

lib/sbi/sbi_tlb.c +3 -0

+ if ((count % 20) == 0) {
+ sbi_dprintf("tlb_sync loop count = %d value = %ld\n",
+    count, atomic_read(tlb_sync));

What value does this add? 20 isn?t that big...

If your goal is to avoid spinning and hammering memory, use pause?

Jess

Quoted 12 lines

lib/sbi/sbi_tlb.c +1 -0

+ }
 }
 
 return;

Signature · 7 lines

2.47.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Bo Gan

2025-01-10 01:20 UTC #2 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Hi Chaoying,

I don't think this patch does anything at all. Using printf to delay is naive. Can you give concrete example of how the livelock happened and when it happens some precise number on the performance impact?

Quoted 36 lines · Jessica Clarke

Quoted 3 lines · Chao-ying Fu
The tlb_sync loop is slowed down by variable latency to avoid livelock on some platforms.
I don?t understand ?by variable latency?. It doesn?t do anything but print.
Quoted 17 lines
---
lib/sbi/sbi_tlb.c | 6 ++++++
1 file changed, 6 insertions(+)
lib/sbi/sbi_tlb.c +1 -0
index 01b31f4..d46b95c 100644
@@ -247,6 +247,7 @@ static void tlb_process(struct sbi_scratch *scratch)
 
 static void tlb_sync(struct sbi_scratch *scratch)
 {
+ int count = 0;
atomic_t *tlb_sync =
sbi_scratch_offset_ptr(scratch, tlb_sync_off);
lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 260
Quoted 4 lines
lib/sbi/sbi_tlb.c +1 -0
@@ -256,6 +257,11 @@ static void tlb_sync(struct sbi_scratch *scratch)
 * consume fifo requests to avoid deadlock.
 */
 tlb_process_once(scratch);
+ count++;
Technically this could overflow, and count is signed, so UB...
lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 263
Quoted 3 lines
lib/sbi/sbi_tlb.c +3 -0
+ if ((count % 20) == 0) {
+ sbi_dprintf("tlb_sync loop count = %d value = %ld\n",
+    count, atomic_read(tlb_sync));

Quoted 20 lines

What value does this add? 20 isn?t that big...
If your goal is to avoid spinning and hammering memory, use pause?
Jess
Quoted 12 lines
lib/sbi/sbi_tlb.c +1 -0
+ }
 }
 
 return;
Signature · 7 lines
2.47.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Chao-ying Fu

author

2025-01-10 02:16 UTC #3 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Quoted 6 lines · On Thu, Jan 9, 2025 at 5:18?PM Bo Gan <[email protected]> wrote:

Hi Chaoying,
I don't think this patch does anything at all. Using printf to delay is naive. Can you give concrete example of how the livelock happened and when it happens some precise number on the performance impact?

Yes, the patch is naive, and we can treat it as the debug patch. However, this patch does help us to avoid livelock.
From our 24-hart configuration, one hart cannot update the variable tlb_sync, because another hart always wins to read the value. Then, the tlb_sync loop never exits.

The performance impact is minimal, I think. But, I don't have precise numbers. Thanks a lot!

Regards,
Chao-ying

Jessica Clarke

2025-01-10 19:08 UTC #4 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Quoted 9 lines · Chao-ying Fu

Quoted 6 lines · On Thu, Jan 9, 2025 at 5:18?PM Bo Gan <[email protected]> wrote:
Hi Chaoying,
I don't think this patch does anything at all. Using printf to delay is naive. Can you give concrete example of how the livelock happened and when it happens some precise number on the performance impact?

Quoted 5 lines

Yes, the patch is naive, and we can treat it as the debug patch. However, this patch does help us to avoid livelock.
From our 24-hart configuration, one hart cannot update the variable tlb_sync, because another hart always wins to read the value. Then, the tlb_sync loop never exits.

That sounds like your core violates the forward progress guarantee required by the RISC-V spec.

Jess

Quoted 5 lines

The performance impact is minimal, I think. But, I don't have precise numbers. Thanks a lot!
Regards,
Chao-ying

Jessica Clarke

2025-01-10 19:12 UTC #5 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Quoted 11 lines · Jessica Clarke

Quoted 9 lines · Chao-ying Fu
Quoted 6 lines · On Thu, Jan 9, 2025 at 5:18?PM Bo Gan <[email protected]> wrote:
Hi Chaoying,
I don't think this patch does anything at all. Using printf to delay is naive. Can you give concrete example of how the livelock happened and when it happens some precise number on the performance impact?

Quoted 8 lines

Quoted 5 lines
Yes, the patch is naive, and we can treat it as the debug patch. However, this patch does help us to avoid livelock.
From our 24-hart configuration, one hart cannot update the variable tlb_sync, because another hart always wins to read the value. Then, the tlb_sync loop never exits.
That sounds like your core violates the forward progress guarantee required by the RISC-V spec.

Except atomic_add_return uses amoadd.d.aqrl, which doesn?t even need such a guarantee, because it?s a single instruction, so that?s saying your hart can wedge on AMOs. Unless you?ve also patched OpenSBI to use LR/SC there, in which case we?re back to the forward progress guarantee being violated.

I have serious concerns about this implementation.

Jess

Chao-ying Fu

author

2025-01-10 02:01 UTC #6 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

In reply to Jessica Clarke

Quoted 8 lines · On Thu, Jan 9, 2025 at 4:52?PM Jessica Clarke <[email protected]> wrote:

Quoted 3 lines · Chao-ying Fu
The tlb_sync loop is slowed down by variable latency to avoid livelock on some platforms.
I don?t understand ?by variable latency?. It doesn?t do anything but print.

This patch serves one purpose to slow down by calculating count % 20 and call the debug printf that may not actually print to the console. Also, we can really use this patch to debug if the tlb_sync loop never exits or runs too long.

Quoted 17 lines

Quoted 17 lines
---
lib/sbi/sbi_tlb.c | 6 ++++++
1 file changed, 6 insertions(+)
lib/sbi/sbi_tlb.c +1 -0
index 01b31f4..d46b95c 100644
@@ -247,6 +247,7 @@ static void tlb_process(struct sbi_scratch *scratch)
 
 static void tlb_sync(struct sbi_scratch *scratch)
 {
+ int count = 0;
atomic_t *tlb_sync =
sbi_scratch_offset_ptr(scratch, tlb_sync_off);

Quoted 7 lines

lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 260
Quoted 5 lines
lib/sbi/sbi_tlb.c +1 -0
@@ -256,6 +257,11 @@ static void tlb_sync(struct sbi_scratch *scratch)
 * consume fifo requests to avoid deadlock.
 */
 tlb_process_once(scratch);
+ count++;
Technically this could overflow, and count is signed, so UB...

We can change to unsigned int.

Quoted 6 lines

lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 263
Quoted 4 lines
lib/sbi/sbi_tlb.c +3 -0
+ if ((count % 20) == 0) {
+ sbi_dprintf("tlb_sync loop count = %d value = %ld\n",
+    count, atomic_read(tlb_sync));
What value does this add? 20 isn?t that big...

This is to slow down the loop more frequently, compared to 1000, for example.

Quoted 4 lines

If your goal is to avoid spinning and hammering memory, use pause?
Jess

Yes, we try to avoid spinning and hammering memory. We will need testing to know if pause can help, too.

Thanks a lot!

Regards,
Chao-ying

Jessica Clarke

2025-01-10 18:55 UTC #7 lore raw

PATCH 1/1 Add a counter and debug printf to change the latency inside tlb_sync

Quoted 8 lines · Chao-ying Fu

Quoted 6 lines · On Thu, Jan 9, 2025 at 4:52?PM Jessica Clarke <[email protected]> wrote:
Quoted 3 lines · Chao-ying Fu
The tlb_sync loop is slowed down by variable latency to avoid livelock on some platforms.

Quoted 7 lines

Quoted 2 lines
I don?t understand ?by variable latency?. It doesn?t do anything but print.
This patch serves one purpose to slow down by calculating count % 20 and call the debug printf that may not actually print to the console. Also, we can really use this patch to debug if the tlb_sync loop never exits or runs too long.

Yeah no. Hard NAK. That?s what pause is for (or wrs.[sn]to).

Jess

Quoted 46 lines

Quoted 16 lines
Quoted 16 lines
---
lib/sbi/sbi_tlb.c | 6 ++++++
1 file changed, 6 insertions(+)
lib/sbi/sbi_tlb.c +1 -0
index 01b31f4..d46b95c 100644
@@ -247,6 +247,7 @@ static void tlb_process(struct sbi_scratch *scratch)
 
 static void tlb_sync(struct sbi_scratch *scratch)
 {
+ int count = 0;
atomic_t *tlb_sync =
sbi_scratch_offset_ptr(scratch, tlb_sync_off);
Quoted 7 lines
lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 260
Quoted 5 lines
lib/sbi/sbi_tlb.c +1 -0
@@ -256,6 +257,11 @@ static void tlb_sync(struct sbi_scratch *scratch)
 * consume fifo requests to avoid deadlock.
 */
 tlb_process_once(scratch);
+ count++;
Technically this could overflow, and count is signed, so UB...
We can change to unsigned int.
Quoted 6 lines
lib/sbi/sbi_tlb.c static void tlb_sync(struct sbi_scratch *scratch) · line 263
Quoted 4 lines
lib/sbi/sbi_tlb.c +3 -0
+ if ((count % 20) == 0) {
+ sbi_dprintf("tlb_sync loop count = %d value = %ld\n",
+    count, atomic_read(tlb_sync));
What value does this add? 20 isn?t that big...
This is to slow down the loop more frequently, compared to 1000, for example.
Quoted 4 lines
If your goal is to avoid spinning and hammering memory, use pause?
Jess
Yes, we try to avoid spinning and hammering memory. We will need testing to know if pause can help, too.
Thanks a lot!
Regards,
Chao-ying