Seemingly excessive use of taskENTER_CRITICAL() and taskEXIT_CRITICAL() macros in the kernel

Kernel
1

I noticed in several places in the kernel source file, that there are seemingly excessive calls of taskENTER_CRITICAL() and taskEXIT_CRITICAL() macros. For instance, inside uxQueueMessagesWaiting():


UBaseType_t uxQueueMessagesWaiting( const QueueHandle_t xQueue )
{
    UBaseType_t uxReturn;

    configASSERT( xQueue );

    taskENTER_CRITICAL();
    {
        uxReturn = ( ( Queue_t * ) xQueue )->uxMessagesWaiting;
    }
    taskEXIT_CRITICAL();

    return uxReturn;
}

And, inside xQueueGetMutexHolder()

    TaskHandle_t xQueueGetMutexHolder( QueueHandle_t xSemaphore )
    {
        TaskHandle_t pxReturn;
        Queue_t * const pxSemaphore = ( Queue_t * ) xSemaphore;

        configASSERT( xSemaphore );

        /* This function is called by xSemaphoreGetMutexHolder(), and should not
         * be called directly.  Note:  This is a good way of determining if the
         * calling task is the mutex holder, but not a good way of determining the
         * identity of the mutex holder, as the holder may change between the
         * following critical section exiting and the function returning. */
        taskENTER_CRITICAL();
        {
            if( pxSemaphore->uxQueueType == queueQUEUE_IS_MUTEX )
            {
                pxReturn = pxSemaphore->u.xSemaphore.xMutexHolder;
            }
            else
            {
                pxReturn = NULL;
            }
        }
        taskEXIT_CRITICAL();

        return pxReturn;
    } /*lint !e818 xSemaphore cannot be a pointer to const because it is a typedef. */

In the first case, why not just to return ( ( Queue_t * ) xQueue )->uxMessagesWaiting, since uxMessagesWaiting has a type of UBaseType_t and supposed to be atomically written and read, as I understand?

In the second case, pxSemaphore->uxQueueType cannot be changed after Semaphore have been created, som again why not to just return pxSemaphore->u.xSemaphore.xMutexHolder, since it have TaskHandle_t type and also, as I understand, have atomic access properties on most architectures. Is the reason lies in the attempt to have universal source code which will work even when those types are not atomically accessible on some MCU/MPU architectures?

2

I agree with your observation in these two cases, and probably wrote the code in the first place. I may be legacy, before the BaseType_t was used here, or possibly because of the pointer access may not be atomic on some architectures. Unless somebody sees something different, I would be happy to receive a pull request that took those critical sections out.

3

If you do raise a PR, here are a couple more candidates in timers.c:

And in queue.c:

And in tasks.c:

4

This change would be very nice but the AWS team is busy on higher priority tasks. I have created this issue: Minimize the use of the taskENTER_CRITICAL and taskEXIT_CRITICAL · Issue #1059 · FreeRTOS/FreeRTOS-Kernel · GitHub for this enhancement and welcome any contributions. The changes should be trivial, but verifying a compiler or architecture does not have any surprises will take some effort.

As @rtel states, the BaseType_t is intended to be atomic but it is possible an architecture (8-bit?) makes the type bigger which will create a problem.

I did glance at the Partner supplied port for AVR MEGA0 and it uses a char for the BaseType_t which is an atomic type. There are other ports to check.