xEventGroupWaitBits broken in FreeRTOS SMP?

Kernel
1

I’m using the RP2040 Port.

From Mastering the FreeRTOS™ Real Time Kernel, “The xEventGroupWaitBits() API Function”,

The calling task specifies bits to test using the uxBitsToWaitFor parameter, and it is likely the
calling task will need to clear these bits back to zero after its unblock condition has been met.
Event bits can be cleared using the xEventGroupClearBits() API function, but using that
function to manually clear event bits will lead to race conditions in the application code if:
 There is more than one task using the same event group.
 Bits are set in the event group by a different task, or by an interrupt service routine.
The xClearOnExit parameter is provided to avoid these potential race conditions. If
xClearOnExit is set to pdTRUE, then the testing and clearing of event bits appears to the
calling task to be an atomic operation (uninterruptable by other tasks or interrupts).

However, if I’m very much mistaken, this test case shows that testing and clearing of event bits is not atomic:

static EventGroupHandle_t xEventGroupHandle;
#define mask 0x01
static TaskHandle_t owner;
#define N 3

static void Task(void *arg) {
    // const EventBits_t mask = 1 << arg;
    unsigned task_no = (unsigned)arg;
    unsigned rand_st = task_no;
    for (;;) {
        EventBits_t xBits =
            xEventGroupWaitBits(xEventGroupHandle, mask, /* uxBitsToWaitFor */
                                pdTRUE,                  /* xClearOnExit */
                                pdFALSE, /* xWaitForAllBits n.a. */
                                portMAX_DELAY);
        configASSERT(mask & xBits);
        configASSERT(0 == owner);
        owner = xTaskGetCurrentTaskHandle();
        if (rand_r(&rand_st) % 2) {
            vTaskDelay(1);
        }
        owner = 0;
        xEventGroupSetBits(xEventGroupHandle, mask);
    }
}

int main(void) {
    stdio_init_all();
    printf("\nHello, world!\n");
    xEventGroupHandle = xEventGroupCreate();
    configASSERT(xEventGroupHandle);
    xEventGroupSetBits(xEventGroupHandle, mask);

    for (size_t i = 0; i < N; ++i) {
        char cTaskName[configMAX_TASK_NAME_LEN];
        snprintf(cTaskName, sizeof(cTaskName), "Task%zu", i);

        BaseType_t xReturned =
            xTaskCreate(Task, cTaskName, 512, (void *)i,
                        configMAX_PRIORITIES -
                            3, /* Priority at which the task is created. */
                        NULL);
        configASSERT(xReturned == pdPASS);
    }

    vTaskStartScheduler();
    /* should never reach here */
    panic_unsupported();
}

gives:

Task2: assertion "0 == owner" failed: file "/home/carlk/pi/pico/FreeRTOS/FreeRTOS/Demo/ThirdParty/Community-Supported/CORTEX_M0+_RP2040/OnEitherCore/main.c", line 27, function: Task

It does not fail if I #define N 2.

Is this a problem in FreeRTOS, or FreeRTOS SMP, or the RP2040 Port? Or am I not very much mistaken?

2

Not sure why you expect owner to be 0 where you do since the definition of an event group signals ALL the tasks waiting on the pattern to be activated. If you start more tasks then you have cores, then the last ones will need to wait for one of the tasks to block to start going, and can thus see the global owner changed.

3

I don’t think it is related to your question, but note there are some updates to the Pi Pico port in this pull request: https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/425

4

I’m going by the description in Mastering the FreeRTOS™ Real Time Kernel.

xClearOnExit
If the calling task’s unblock condition has been met, and xClearOnExit is
set to pdTRUE, then the event bits specified by uxBitsToWaitFor will be
cleared back to 0 in the event group before the calling task exits the
xEventGroupWaitBits() API function.

If xClearOnExit is set to pdTRUE, then the testing and clearing of event bits appears to the
calling task to be an atomic operation (uninterruptable by other tasks or interrupts).

From that I inferred that if xClearOnExit is set to pdTRUE in each of the waiting tasks, exactly one task can get unblocked when xEventGroupSetBits() sets the bit they are waiting for. Maybe a bad interpretation on my part.

The reason that I ask is that the code for FF_LockFAT and FF_UnlockFAT, for example, in Lab-Project-FreeRTOS-FAT’s ff_locking.c seem to me to depend on that behavior.

5

To elaborate, the current ff_locking.c might be broken because of a change that I suggested a couple years ago: Comparing c50b4b9..97e608c · FreeRTOS/Lab-Project-FreeRTOS-FAT · GitHub

If I change my testcase to look more like the original FF_LockFAT:

static void Task(void *arg) {
    unsigned task_no = (unsigned)arg;
    unsigned rand_st = task_no;
    for (;;) {
        EventBits_t xBits =
            xEventGroupWaitBits(xEventGroupHandle, 
                                mask, /* uxBitsToWaitFor */
                                pdFALSE, /* xClearOnExit */
                                pdFALSE, /* xWaitForAllBits n.a. */
                                portMAX_DELAY);
        xBits = xEventGroupClearBits(xEventGroupHandle, mask);
        if (mask & xBits) {
            /* This task has cleared the desired bit.
			It now 'owns' the resource. */
            configASSERT(0 == owner);
            owner = xTaskGetCurrentTaskHandle();
            printf("%s owns the resource\n", pcTaskGetName(NULL));
            if (rand_r(&rand_st) % 2) {
                vTaskDelay(1);
            }
            owner = 0;
            xEventGroupSetBits(xEventGroupHandle, mask);
        };
    }
}

it actually runs fine, but I don’t understand why. I guess the xEventGroupWaitBits releases all waiting tasks, but only one will see a 1 in xBits after xEventGroupClearBits?

So, there is a meaningful difference between

        EventBits_t xBits =
            xEventGroupWaitBits(xEventGroupHandle, 
                                mask, /* uxBitsToWaitFor */
                                pdTRUE,                  /* xClearOnExit */
                                pdFALSE, /* xWaitForAllBits n.a. */
                                portMAX_DELAY);

and

        EventBits_t xBits =
            xEventGroupWaitBits(xEventGroupHandle, 
                                mask, /* uxBitsToWaitFor */
                                pdFALSE, /* xClearOnExit */
                                pdFALSE, /* xWaitForAllBits n.a. */
                                portMAX_DELAY);
        xBits = xEventGroupClearBits(xEventGroupHandle, mask);

?

6

That is incorrect. When xEventGroupSetBits is called, EVERY task whose condition is now met is unblocked, and given the return code of the bits at that point, and THEN the bits are cleared if requested by any of the tasks.

This WHOLE operation is atomic, not step by step atomic.

7

Thanks! It is all clear to me now.

Henry David Thoreau said “Simplify, simplify, simplify!”, but he obviously didn’t know what he was talking about: his own statement is highly redundant and could be simplified. Unless… Was he warning about the hazards of oversimplifying?

Oh well, as Red Green says, “If it ain’t broke, you’re not tryin’ hard enough.”

8

Is that requirement really met by this implementation of xEventGroupWaitBits?

If it clears the bits while in vTaskSuspendAll how will the other tasks get unblocked?

9

Note, that waitbits it’s only clears the bits itself, if it finds the condition true on entry, at which point there can’t be any other tasks still waiting on that condition.

It is setbits that wakes up all the tasks and then clears all the bits that were marked to be cleared by calls to waitbits, and it does that by looping over all the tasks waiting on the group, waking up those now satisfied, and accumulating what bits need to be cleared, and then AFTER doing that loop, clears the requested bits.

10

I see. Thanks for the explanation.

So, for example, in the context of LockFAT:

  1. Task1 enters xEventGroupWaitBits. It finds the FF_FAT_LOCK_EVENT_BITS bit set and there is no contention. It clears the bit by itself.
  2. Task2 enters xEventGroupWaitBits. It finds the bit cleared, and it puts itself on the wait list with vTaskPlaceOnUnorderedEventList. Later, it will return to the following statement in the code.
  3. Task3 enters xEventGroupWaitBits. It finds the bit cleared, and it puts itself on the wait list with vTaskPlaceOnUnorderedEventList. Later, it will return to the following statement in the code (well past where it would clear the bit itself).
  4. Task1 calls xEventGroupSetBits, which iterates over the EventList, unblocking Task2 and Task3. If xClearOnExit were set to pdTRUE, it would clear the bit.
  5. The scheduler resumes Task2 and/or Task3.

Am I wrong?

11

In step 4, Task1 sets the bit, which unlocks BOTH task2 and task3. For the locking to work xClearOnExit CAN’T be true or neither of them has assurance they are the only task running, since they aren’t.

Then for step 5 whichever task FIRST gets to the ClearBits call first, sees that it cleared the bit, so can continue.

The other task(s) when they get to the ClearBits call, sees that the bit was already clear, so they go back to the xEventGroupWaitBits call.

So in this case, the ACTUAL locking is being done by the xEventGroupClearBits call, the whole purpose of the xEventGroupWaitBits call is to not use CPU time when we know we can not succeed at the clearbits call.

This is the key difference between the auto-clear option and a discrete clear call, with the auto clear option, ALL tasks that were waiting will see the bit set, so we don’t have exclusiveness, but we can have the event be broadcast. With the xEventGroupClearBits, only 1 task can get the bit returned until someone calls xEventGroupSetBits again, so it can be a form of exclusion.

12

Excellent. Thanks again!

I had never really looked at the event_groups.c source code before. I do think the documentation could be a little better in this area. For example, xEventGroupSetBits() says

Setting bits in an event group will automatically unblock tasks that are blocked waiting for the bits.

To me, it would be clearer if it said

Setting bits in an event group will unblock all tasks that are blocked waiting for those bits.

(I don’t know what the word “automatically” means in that sentence, and it doesn’t say how many will be unblocked.)

Mastering the FreeRTOS™ Real Time Kernel says in regards to The xEventGroupWaitBits() API Function

If xClearOnExit is set to pdTRUE, then the testing and clearing of event bits appears to the
calling task to be an atomic operation (uninterruptable by other tasks or interrupts).

To me, it would be clearer if it said… well, I’m not sure how I would reword that. I just find it confusing.

13

I wanted to make sure how the old and the newer locking methods behave on a single-core CPU, e.g. an STM32F7xx.

Please find here my testing code:
event_group_tests.c (2.4 KB)

The module will start 5 tasks with the idle priority that will continuously try to “lock” a device, wait for a random time, and release it.

Here is the essence of it:

for( ;; )
{
    #if( USE_METHOD == OLD_METHOD )
        uxRc = xEventGroupWaitBits( xGroup,
                                    MY_BIT,   // uxBitsToWaitFor
                                    pdFALSE,  // xClearOnExit
                                    pdFALSE,  // xWaitForAllBits
                                    pdMS_TO_TICKS( 10000U ) );
        uxRc = xEventGroupClearBits( xGroup, MY_BIT );
    #else
        uxRc = xEventGroupWaitBits( xGroup,
                                    MY_BIT,   // uxBitsToWaitFor
                                    pdTRUE,   // xClearOnExit
                                    pdFALSE,  // xWaitForAllBits
                                    pdMS_TO_TICKS( 10000U ) );
    #endif
    if( ( uxRc & MY_BIT ) != 0U )
    {
        configASSERT( xOwner == 0 );
        xOwner = xIndex;
        break;
    }
}

With the new method, things went wrong immediately. I will approve Carl’s PR.

Thanks