taskENTER_CRITICAL vs vTaskSuspendAll in uxStreamBufferAdd

Hi,

I wonder why the function uxStreamBufferAdd in FreeRTOS_Stream_Buffer.c uses vTaskSuspendAll instead of taskENTER_CRITICAL.
That protected region seems to be very short and time bounded so vTaskSuspendAll may be overkill and potentially expensive when resuming the scheduler.

I know taskENTER_CRITICAL will disable interrupts but given the region is just a few time-bound operations we should probably be fine and save quite a bit of CPU in a frequently used function such as uxStreamBufferAdd.

BTW, stream_buffer.c uses mostly taskENTER_CRITICAL for those types of sections.

Thanks!

1 Like

The code you mention is here. From what I can see you are right.

Using taskENTER_CRITICAL looks like a good change in uxStreamBufferAdd.

We would welcome a PR making that change.

Done here: Optimize uxStreamBufferAdd() locking mechanism by lzungri · Pull Request #1181 · FreeRTOS/FreeRTOS-Plus-TCP · GitHub

I think there are many other places where taskENTER_CRITICAL may be a better fit as well. For example:

To name a few.

I think potentially unbound list operations are no good candidates for using critical sections. That would induce non-deterministic, runtime dependent ISR jitter which is not acceptable.
However, some of your findings are worth to be considered. Thanks for your effort !

It is important that any use of taskENTER_CRITICAL be in tight, bounded blocks of code. Any performance improvement to using taskEXIT_CRITICAL is only realized if the tick timer interrupt DOES NOT fire and all system level interrupts are delayed while the critical section is active.

vTaskSuspendAll will leave all interrupts active and only disable the scheduler so when the tick timer interrupt fires, the scheduler is skipped. The scheduler is re-run on vTaskResumeAll which is the source of slower operation. This keeps interrupts active at the cost of ALWAYS running the scheduler and slowing down this type of critical section.

Interrupts that use a FreeRTOS command such as xQueueSendFromISR will still delay the scheduler and not wake any higher priority tasks that are waiting for the data. When the vTaskResumeAll eventually happens, the higher priority task will be scheduled. This can have the affect of priority inversion as a high priority task will continue to the blocked while a low priority task has stopped the scheduler. BE CAREFUL. Sometimes a semaphore/mutex is a better choice especially for low priority tasks.

All things are a tradeoff and this is a great discussion.