Optimize uxStreamBufferAdd() locking mechanism

[lzungri]

To optimize performance, consider replacing the potentially costly vTaskSuspendAll/xTaskResumeAll locking mechanism with the more efficient taskENTER_CRITICAL/taskEXIT_CRITICAL pair.
While taskENTER_CRITICAL disables interrupts, the protected code section is relatively short and time-bound, making it a suitable candidate. This is especially advantageous given the high-frequency usage of uxStreamBufferAdd().

[tony-josi-aws]

@lzungri

Thanks for taking time to contribute to FreeRTOS+TCP.
We will do some performance analysis with this change and get back to you.

[htibosch]

Hello @lzungri, thank you for presenting your PR.

In order to make a critical section, this library prefers to suspend the scheduler, rather than masking interrupts.

The library is used on many platforms, and often, FreeRTOS users want an optimal ISR response time by masking interrupts as little and as shortly as possible.

It must be said that xTaskResumeAll() will call taskENTER_CRITICAL(), to test if it needs to yield and do a task-switch.

Tony wrote:

We will do some performance analysis with this change and get back to you.

I'm curious about that test.

Thanks

[lzungri]

We will do some performance analysis with this change and get back to you.

Thanks for taking a look at it.

The library is used on many platforms, and often, FreeRTOS users want an optimal ISR response time by masking interrupts as little and as shortly as possible.

Agreed. If used incorrectly, taskENTER_CRITICAL can lead to performance degradation. However, in this particular instance, given the protected region is short and time-bound we should see a performance improvement. The kernel also favors taskENTER_CRITICAL for these type of cases (e.g. https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/a045081f73533ca972f0d9f8e23bf3ea61709b7d/stream_buffer.c#L875).

Speaking of which, there are several other places where I think taskENTER_CRITICAL might be a better fit as well, specially when dealing with linked list operations that don't traverse the list and just update a few pointers such as uxListRemove, listGET_OWNER_OF_HEAD_ENTRY, and others:

• FreeRTOS-Plus-TCP/source/FreeRTOS_Sockets.c

  Line 1174 in d701079

  vTaskSuspendAll();

• FreeRTOS-Plus-TCP/source/FreeRTOS_Sockets.c

  Line 1858 in d701079

  vTaskSuspendAll();

• FreeRTOS-Plus-TCP/source/FreeRTOS_Sockets.c

  Line 2130 in d701079

  vTaskSuspendAll();

• FreeRTOS-Plus-TCP/source/FreeRTOS_Sockets.c

  Line 3890 in d701079

  vTaskSuspendAll();

  (we should remove the FreeRTOS_printf here)

• FreeRTOS-Plus-TCP/source/FreeRTOS_Sockets.c

  Line 4534 in d701079

  vTaskSuspendAll();

Just to name a few. Maybe this should be part of a bigger "Performance improvements" PR.

[shubnil]

I see the choice depends on a combination of 3 factors:

1. Time spent in critical section
2. How frequently the critical section code is invoked
3. Does the critical section needs to complete fast and re-enable the interrupts
   We did some experiments on this to understand the differences in behavior corresponding to this PR and are in the process of collating the data. We will share the same soon.

[tony-josi-aws]

@lzungri

The changes look good; disabling interrupts is faster than suspending the scheduler and resuming it back.

Also, are you willing to make similar changes (that you have listed in the previous comment) where the protected region is short and time-bound as well, maybe in a different PR or as part of this PR?

[lzungri]

Also, are you willing to make similar changes (that you have listed in the previous comment) where the protected region is short and time-bound as well, maybe in a different PR or as part of this PR?

@tony-josi-aws I may be able to work on that in a week or two (in a new PR).

[tony-josi-aws]

@lzungri

Thank you.