codec2_vndk: don't spin during sync variable locking/unlocking
Removed spinning and sched_yield for the user-space locking section
of lock/unlock and added comments to the code.
TODO: remove the spinning code in unlock as it does not seem to be
necessary.
Bug: 287432155
(cherry picked from https://android-review.googlesource.com/q/commit:6c902ef92de5fa8ae4a1c12dd67e95ac5a8e5935)
Merged-In: Ie283402b3177013f918e3facb287c23a13df0550
Change-Id: Ie283402b3177013f918e3facb287c23a13df0550
diff --git a/media/codec2/vndk/platform/C2SurfaceSyncObj.cpp b/media/codec2/vndk/platform/C2SurfaceSyncObj.cpp
index d55a3d8..d8c2292 100644
--- a/media/codec2/vndk/platform/C2SurfaceSyncObj.cpp
+++ b/media/codec2/vndk/platform/C2SurfaceSyncObj.cpp
@@ -114,8 +114,8 @@
}
namespace {
- constexpr int kSpinNumForLock = 100;
- constexpr int kSpinNumForUnlock = 200;
+ constexpr int kSpinNumForLock = 0;
+ constexpr int kSpinNumForUnlock = 0;
enum : uint32_t {
FUTEX_UNLOCKED = 0,
@@ -125,32 +125,65 @@
}
int C2SyncVariables::lock() {
- uint32_t old;
+ uint32_t old = FUTEX_UNLOCKED;
+
+ // see if we can lock uncontended immediately (if previously unlocked)
+ if (mLock.compare_exchange_strong(old, FUTEX_LOCKED_UNCONTENDED)) {
+ return 0;
+ }
+
+ // spin to see if we can get it with a short wait without involving kernel
for (int i = 0; i < kSpinNumForLock; i++) {
- old = 0;
+ sched_yield();
+
+ old = FUTEX_UNLOCKED;
if (mLock.compare_exchange_strong(old, FUTEX_LOCKED_UNCONTENDED)) {
return 0;
}
- sched_yield();
}
- if (old == FUTEX_LOCKED_UNCONTENDED)
+ // still locked, if other side thinks it was uncontended, now it is contended, so let them
+ // know that they need to wake us up.
+ if (old == FUTEX_LOCKED_UNCONTENDED) {
old = mLock.exchange(FUTEX_LOCKED_CONTENDED);
+ // It is possible that the other holder released the lock at this very moment (and old
+ // becomes UNLOCKED), If so, we will not involve the kernel to wait for the lock to be
+ // released, but are still marking our lock contended (even though we are the only
+ // holders.)
+ }
- while (old) {
- (void) syscall(__NR_futex, &mLock, FUTEX_WAIT, FUTEX_LOCKED_CONTENDED, NULL, NULL, 0);
+ // while the futex is still locked by someone else
+ while (old != FUTEX_UNLOCKED) {
+ // wait until other side releases the lock (and still contented)
+ (void)syscall(__NR_futex, &mLock, FUTEX_WAIT, FUTEX_LOCKED_CONTENDED, NULL, NULL, 0);
+ // try to relock
old = mLock.exchange(FUTEX_LOCKED_CONTENDED);
}
return 0;
}
int C2SyncVariables::unlock() {
- if (mLock.exchange(FUTEX_UNLOCKED) == FUTEX_LOCKED_UNCONTENDED) return 0;
+ // TRICKY: here we assume that we are holding this lock
+ // unlock the lock immediately (since we were holding it)
+ // If it is (still) locked uncontested, we are done (no need to involve the kernel)
+ if (mLock.exchange(FUTEX_UNLOCKED) == FUTEX_LOCKED_UNCONTENDED) {
+ return 0;
+ }
+
+ // We don't need to spin for unlock as here we know already we have a waiter who we need to
+ // wake up. This code was here in case someone just happened to lock this lock (uncontested)
+ // before we would wake up other waiters to avoid a syscall. It is unsure if this ever gets
+ // exercised or if this is the behavior we want. (Note that if this code is removed, the same
+ // situation is still handled in lock() by the woken up waiter that realizes that the lock is
+ // now taken.)
for (int i = 0; i < kSpinNumForUnlock; i++) {
- if (mLock.load()) {
+ // here we seem to check if someone relocked this lock, and if they relocked uncontested,
+ // we up it to contested (since there are other waiters.)
+ if (mLock.load() != FUTEX_UNLOCKED) {
uint32_t old = FUTEX_LOCKED_UNCONTENDED;
mLock.compare_exchange_strong(old, FUTEX_LOCKED_CONTENDED);
+ // this is always true here so we return immediately
if (old) {
return 0;
}
@@ -158,7 +191,8 @@
sched_yield();
}
- (void) syscall(__NR_futex, &mLock, FUTEX_WAKE, 1, NULL, NULL, 0);
+ // wake up one waiter
+ (void)syscall(__NR_futex, &mLock, FUTEX_WAKE, 1, NULL, NULL, 0);
return 0;
}