Revert "[scudo] Use getMonotonicTimeFast for tryLock."

[ChiaHungDuan]

This reverts commit 36ca9a2.

We were using the time as the seed while choosing a new TSD. To make the access of TSDs evenly distributed, we require a higher precision in time. Otherwise, many threads may result in having the same random access pattern on TSDs because they share the same time in certain period. On Linux, CLOCK_MONOTONIC_COARSE usually adopts 4 ms precision. This is way higher than the average accessing time of TSD (which is usually less than 1 us). As a result, when multiple threads try to select a new TSD in a 4 ms interval, they share the same time seed and end up choosing and congesting on the same TSD.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (ChiaHungDuan)

Changes

This reverts commit 36ca9a2.

We were using the time as the seed while choosing a new TSD. To make the access of TSDs evenly distributed, we require a higher precision in time. Otherwise, many threads may result in having the same random access pattern on TSDs because they share the same time in certain period. On Linux, CLOCK_MONOTONIC_COARSE usually adopts 4 ms precision. This is way higher than the average accessing time of TSD (which is usually less than 1 us). As a result, when multiple threads try to select a new TSD in a 4 ms interval, they share the same time seed and end up choosing and congesting on the same TSD.

---

Full diff: https://github.com/llvm/llvm-project/pull/86590.diff

1 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/tsd.h (+3-3)

diff --git a/compiler-rt/lib/scudo/standalone/tsd.h b/compiler-rt/lib/scudo/standalone/tsd.h
index b2108a01900bcc..72773f2f72b116 100644
--- a/compiler-rt/lib/scudo/standalone/tsd.h
+++ b/compiler-rt/lib/scudo/standalone/tsd.h
@@ -41,9 +41,9 @@ template <class Allocator> struct alignas(SCUDO_CACHE_LINE_SIZE) TSD {
       return true;
     }
     if (atomic_load_relaxed(&Precedence) == 0)
-      atomic_store_relaxed(&Precedence,
-                           static_cast<uptr>(getMonotonicTimeFast() >>
-                                             FIRST_32_SECOND_64(16, 0)));
+      atomic_store_relaxed(
+          &Precedence,
+          static_cast<uptr>(getMonotonicTime() >> FIRST_32_SECOND_64(16, 0)));
     return false;
   }
   inline void lock() NO_THREAD_SAFETY_ANALYSIS {

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cferris1000]

We could also continue to use this function if we did a right shift to get better resolution.

However, I'm not sure that using a random seed here make a lot of sense. It almost might be better to just use a round-robin especially when there are only two TSDs.

But that requires more experimentation.

[ChiaHungDuan]

We could also continue to use this function if we did a right shift to get better resolution.

However, I'm not sure that using a random seed here make a lot of sense. It almost might be better to just use a round-robin especially when there are only two TSDs.

But that requires more experimentation.

In this case, for example, getMonotonicTimeFast() in the interval of 0~4 ms returns the same value, so the shifting still gives the same value. I did some experiments with different strategies, it has very small improvements but I think it's still worth of adopting new algorithm.

[cferris1000]

We could also continue to use this function if we did a right shift to get better resolution.
However, I'm not sure that using a random seed here make a lot of sense. It almost might be better to just use a round-robin especially when there are only two TSDs.
But that requires more experimentation.

In this case, for example, getMonotonicTimeFast() in the interval of 0~4 ms returns the same value, so the shifting still gives the same value. I did some experiments with different strategies, it has very small improvements but I think it's still worth of adopting new algorithm.

Yeah, I'm not sure what the original intent of making this random was. I don't think it makes anything more secure doing this, especially if there are only two TSDs. The more TSDs there are, then doing something more random is probably a better strategy, so whatever we do might need to incorporate the number of TSDs.

[ChiaHungDuan]

We could also continue to use this function if we did a right shift to get better resolution.
However, I'm not sure that using a random seed here make a lot of sense. It almost might be better to just use a round-robin especially when there are only two TSDs.
But that requires more experimentation.

In this case, for example, getMonotonicTimeFast() in the interval of 0~4 ms returns the same value, so the shifting still gives the same value. I did some experiments with different strategies, it has very small improvements but I think it's still worth of adopting new algorithm.

Yeah, I'm not sure what the original intent of making this random was. I don't think it makes anything more secure doing this, especially if there are only two TSDs. The more TSDs there are, then doing something more random is probably a better strategy, so whatever we do might need to incorporate the number of TSDs.

Agree, let's do it in a follow up CL