Why check pxCond->xIsInitialized flag twice and use critical section to prevent race condition

[Saiiijchan]

Hi, I am try to port POSIX lab to v11.1 SMP.

I meet a problem when test pthread condition

Lab-Project-FreeRTOS-POSIX/FreeRTOS-Plus-POSIX/source/FreeRTOS_POSIX_pthread_cond.c

Line 64 in f1fe687

taskENTER_CRITICAL();

.

The pvPortMalloc() will call vTaskSuspendAll() but vTaskSuspendaLL() CAN not be called in critical secion.

Besides, I don't understand why check pxCond->xIsInitialized flag twice on line 60 and 69.

[rlshumat]

@Saiiijchan As you have noted, the POSIX pthread_cond implementation did not take into account SMP. vTaskSuspendAll() in a single core system can be called in a critical section. To resolve this for SMP, instead of calling taskENTER_CRITCIAL() in prvInitializeStaticCond calling vTaskSuspendAll() should be an acceptable solution as we are really only concerned with a race condition between two tasks initializing the pthread_cond and if the schedular is suspended, a race should never occur since the POSIX API is not ISR safe and should not be used there anyways.

Regarding the double check of the IsInitialized variable is due to preemption. Should two tasks be attempting to initialize the pthread_cond at the same time, if the preemption occurs after both tasks have passed the initial check then there is potential that both tasks could attempt to initialize the pthread_cond. While this is very unlikely to occur, we still need to test for it. The initial check prevents us from entering the critical section if the condition has already be initialized.

[Saiiijchan]

Thanks for your answer.

instead of calling taskENTER_CRITCIAL() in prvInitializeStaticCond calling vTaskSuspendAll() should be an acceptable solution as we are really only concerned with a race condition between two tasks initializing the pthread_cond and if the schedular is suspended, a race should never occur since the POSIX API is not ISR safe and should not be used there anyways.

Since suspending the scheduler (via vTaskSuspendAll()) can prevent race conditions, could we simply use vTaskSuspendAll() instead of taskENTER_CRITICAL() no matter on single-core or SMP systems? After all, vTaskSuspendAll() has lower overhead.

[rlshumat]

I may be missing something, but from my analysis it should be sufficient for both to call vTaskSuspendAll() instead of taskENTER_CRITICAL(). It is worth testing in my opinion. Maybe one of the FreeRTOS devs could provide their opinion? @aggarg

[aggarg]

@rlshumat, @Saiiijchan - Thank you for sharing your analysis. I agree that we can change to vTaskSuspendAll().

if the preemption occurs after both tasks have passed the initial check then there is potential that both tasks could attempt to initialize the pthread_cond.

Regarding the above point, this second check should prevent it, right?

[Saiiijchan]

@aggarg @rlshumat I pushed a PR.
If there are existing test cases for POSIX pthread_cond, I can execute them to verify the behavior.

[aggarg]

No, we do not have any test case unfortunately. You are welcome to contribute though :)