prvResetNextTaskUnblockTime and vTaskRemoveFromUnorderedEventList

jyates-au wrote on Friday, October 25, 2019:

I have been debugging an issue that appears when utilising event groups together with Tickless idle.
When waiting on bits with a timeout, xNextTaskUnblockTime is not being updated when the bits are set.
Therefore the system wakes up again at the time the timeout would have expired, despite there being no reason to do so.
xTaskRemoveFromEventList calls prvResetNextTaskUnblockTime to avoid this issue, which covers queues and their derivitives.
vTaskRemoveFromUnorderedEventList however does not, which is the variant event groups use.
I have provisionally fixed the issue by copying the call from xTaskRemoveFromEventList to vTaskRemoveFromUnorderedEventList.
I am however not sure whether this is an oversight, or whether there is a valid reason for it to be missing.
If there is a valid reason, there is no explanatory documentation I can find.
Confirmation that this is a bug, or an unfortunate side effect due to some synchronisation issue that I am unaware of would be greatly appreciated.
J Yates

rtel wrote on Friday, October 25, 2019:

From your description it sounds like a bug, so we will investigate
further and fix if confirmed. Grateful if you could attached your
update to the forum post - thanks.

jyates-au wrote on Saturday, October 26, 2019:

Hi Richard,

Thanks for your response. Reviewing the changes made to xTaskRemoveFromEventList in v10.2.0 made me look more deeply at the logic flow.
The problem is still the same, prvResetNextTaskUnblockTime not being called appropriately, but my proposed solution didn’t fix the root cause.
The following is the abbreviated logic flow for xEventGroupSetBits when the bits are unblocking another task.

  1. xEventGroupSetBits calls vTaskSuspendAll (event_groups.c:535)
  2. xEventGroupSetBits calls vTaskRemoveFromUnorderedEventList (event_groups.c:594)
    1. vTaskRemoveFromUnorderedEventList calls prvAddTaskToReadyList (tasks.c:3183)
      1. prvAddTaskToReadyList macro adds the previously blocking task DIRECTLY to pxReadyTasksLists (tasks.c:221)
  3. xEventGroupSetBits then calls xTaskResumeAll (event_groups.c:607)
    1. xTaskResumeAll pulls the highest priority task from xPendingReadyList (tasks.c:2200)
    2. If there was a task on xPendingReadyList, it calls prvResetNextTaskUnblockTime (tasks.c:2225)
      Per the commment, this is the correct place to handle tasks that were unblocked while the scheduler was suspended (the situtation with event groups)
      This never runs in the case of event groups case because there is never anything on xPendingReadyList

In contrast, the queue implementation

  1. xQueueGenericSend calls xTaskRemoveFromEventList (queue.c:846)
    1. xTaskRemoveFromEventList conditionally either calls prvAddTaskToReadyList or adds the task to xPendingReadyList depending on the scheduler state (tasks.c:3116-3140)

The problem therefore appears to be that vTaskRemoveFromUnorderedEventList is simply adding the task to the wrong queue given its precondition of the scheduler being suspended.
It should be adding the task to xPendingReadyList, but is instead adding it to pxReadyTasksLists.
My proposed solution is therefore replacing tasks.c:3179-3183 with tasks:3137-3139

/* The delayed and ready lists cannot be accessed, so hold this task
pending until the scheduler is resumed. */
vListInsertEnd( &( xPendingReadyList ), &( pxUnblockedTCB->xEventListItem ) );

I have tested this change locally and it resolves the unwaranted wakeups, however I obviously can’t gaurantee it doesn’t break anything.
Logically the solution lines up with the existing documentation, whereas the existing code doesn’t.


jyates-au wrote on Monday, November 04, 2019:

Hi Richard,
Any updates on reviewing my findings?

rtel wrote on Monday, November 04, 2019:

Sure - looks like you are right - apologies for not posting back. This will get updated. Great if you could attach what you did to the forum post so we can compare.

jyates-au wrote on Monday, November 04, 2019:

No problem. Here is the modified source.

jyates-au wrote on Thursday, November 14, 2019:

Another update, the fix as proposed doesn’t respect the requirements of xPendingReadyList.
From the comment at tasks.c:378, xPendingReadyList can only be modified in a critical section.
xTaskRemoveFromEventList has the precondition that it must be called from a critical section, whereas vTaskRemoveFromUnorderedEventList does not.
Unfortunately neither of the locations vTaskRemoveFromUnorderedEventList are called are currently inside critical sections.
Therefore the condition for modifying xPendingReadyList is violated [ Causing annoyingly rare hard faults ].
Either the critical section pre-condition must be added to the function, and callers updated, or the vListInsertEnd call must be wrapped in a critical section.
Given that the function didn’t previously require a critical section, I don’t believe the rest of it requires it now.
I’ll let you decide which solution makes for sense in the context of that larger codebase, i’ll just be wrapping the vListInsertEnd call for now.

Again, confirmation that my findings are correct would be appreciated.

Jordan Yates