Skip to content

Commit 4648578

Browse files
committed
OS/FreeRTOS: Correct recursive spinlock for weak memory models in FreeRTOS SMP
The previous implementation of `vPortRecursiveLock` was unsafe on systems with weak memory ordering (like RISC-V), leading to potential race conditions. This commit corrects the implementation by introducing necessary memory barriers and optimizes the spin-wait loop. **Correctness Fixes:** * **Acquire Barrier**: An acquire memory barrier (`__RWMB()`) is added immediately after a successful atomic swap (`__AMOSWAP_W`). This is critical to prevent the compiler or CPU from reordering memory operations from within the critical section to before the lock is actually acquired. Without this, the lock provides no protection. * **Release Barrier**: A release memory barrier (`__RWMB()`) is added before the lock variable is cleared. This ensures that all memory writes within the critical section are globally visible *before* the lock is released. This prevents other cores from acquiring the lock and seeing stale data. **Performance and Logic Improvements:** * **Test-and-Test-and-Set (TTS)**: The lock acquisition logic has been restructured into a more efficient TTS pattern. The code now spins on a cheap, non-atomic read (`while (*pxSpinLock == 0)`) and only attempts the expensive atomic swap when the lock appears to be free. This significantly reduces bus contention and improves system performance when multiple cores are contending for a lock. * **NOP in Spin Loop**: A `__NOP()` has been added to the spin-wait loop. This can help reduce power consumption and pipeline pressure on some CPU architectures during tight spins. * **Improved Readability**: Added comments to clarify the logic for recursive locking, lock acquisition, and the purpose of the memory barriers. Signed-off-by: Huaqi Fang <578567190@qq.com>
1 parent 029689f commit 4648578

1 file changed

Lines changed: 22 additions & 11 deletions

File tree

  • OS/FreeRTOS/Source/portable

OS/FreeRTOS/Source/portable/port.c

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -176,42 +176,53 @@ spin_lock_t hw_sync_locks[portRTOS_SPINLOCK_COUNT] = {0, 0};
176176
* uxAcquire, and the compiler should do the right thing! */
177177
void vPortRecursiveLock(unsigned long ulLockNum, spin_lock_t *pxSpinLock, BaseType_t uxAcquire)
178178
{
179+
/* Track, per-core, which locks this core currently owns. */
179180
static uint8_t ucOwnedByCore[portMAX_CORE_COUNT];
181+
/* Track, per-lock, how many times it has been recursively taken. */
180182
static uint8_t ucRecursionCountByLock[portRTOS_SPINLOCK_COUNT];
181183

182184
configASSERT(ulLockNum < portRTOS_SPINLOCK_COUNT);
183-
unsigned long ulCoreNum = __get_hart_index();
184-
unsigned long ulLockBit = 1u << ulLockNum;
185+
unsigned long ulCoreNum = __get_hart_index(); /* ID of current hart */
186+
unsigned long ulLockBit = 1u << ulLockNum; /* Bit mask for lock */
185187
configASSERT(ulLockBit < 256u);
186188

187-
if (uxAcquire) {
189+
if (uxAcquire) { /* ACQUIRE PATH */
190+
/* Case 1: lock already held by THIS core -> pure recursion. */
188191
if ((!*pxSpinLock == 0)) {
189192
if (ucOwnedByCore[ulCoreNum] & ulLockBit) {
190193
configASSERT(ucRecursionCountByLock[ulLockNum] != 255u);
191194
ucRecursionCountByLock[ulLockNum]++;
192195
return;
193196
}
194-
195-
while ((!*pxSpinLock == 0)) {
196-
}
197197
}
198198

199+
/* Case 2: lock not held (or held by another core). */
199200
do {
200-
if (__AMOSWAP_W(pxSpinLock, 1) == 0) {
201-
break;
201+
/* Spin-wait until the lock appears free. */
202+
while ((!*pxSpinLock == 0)) {
203+
__NOP();
204+
}
205+
/* Atomically attempt to take the lock. */
206+
if (__AMOSWAP_W(pxSpinLock, 1) == 0) { /* success */
207+
__RWMB(); /* mem-barrier */
208+
break; /* lock taken */
202209
}
203210
} while (1);
204211

212+
/* First successful take on THIS core -> init recursion state.*/
205213
configASSERT(ucRecursionCountByLock[ulLockNum] == 0);
206214
ucRecursionCountByLock[ulLockNum] = 1;
207-
ucOwnedByCore[ulCoreNum] |= ulLockBit;
208-
} else {
215+
ucOwnedByCore[ulCoreNum] |= ulLockBit; /* mark ownership */
216+
} else { /* RELEASE PATH */
209217
configASSERT((ucOwnedByCore[ulCoreNum] & ulLockBit) != 0);
210218
configASSERT(ucRecursionCountByLock[ulLockNum] != 0);
211219

220+
/* Decrease recursion counter. */
212221
if (!--ucRecursionCountByLock[ulLockNum]) {
222+
/* Last release -> clear ownership and unlock. */
213223
ucOwnedByCore[ulCoreNum] &= ~ulLockBit;
214-
*pxSpinLock = 0;
224+
__RWMB(); /* ensure prior stores visible */
225+
*pxSpinLock = 0; /* hand the lock back */
215226
}
216227
}
217228
}

0 commit comments

Comments
 (0)