search

FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.52k stars 1.05k forks source link

Fix prvSelectHighestPriorityTask ready list iteration #991

Closed gemarcano closed 4 months ago

gemarcano commented 5 months ago

Fix prvSelectHighestPriorityTask ready list iteration. Fixes #990.

Description

Use listGET_OWNER_OF_NEXT_ENTRY to iterate through the ready task list instead of iterating from the head of the list. The SMP changes to prvSelectHighestPriorityTask added the use of vListInsertEnd to append the current task to the end of the list. vListInsertEnd, however, appends a node such that, when using listGET_OWNER_OF_NEXT_ENTRY, it is the last node returned, but it isn't inserted at the actual tail of the list. This led to a case where the list's pxIndex eventually managed to become the head of the list, and using vListInsertEnd would cause the added node to be added to the head of the list.

Test Steps

This is not very deterministic to reproduce, but I had around 7 tasks being scheduled, and many of them were sleeping for different amounts of time. The main cause seems to be the pxIndex of the ready lists moving to the front of the list.

Checklist:

Related Issue

990

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

gemarcano commented 5 months ago

I'm not sure how to go about running regression tests and/or how to write unit tests for this, but I'd be happy to try if pointed in the right direction. The problem is I'm not sure how to reproduce the original issue reliably.

rawalexe commented 5 months ago

Thank you for your contribution, we are working towards reviewing your PR.

gemarcano commented 5 months ago

I noticed the failed formatting and warning checks-- I've gone ahead and updated the commit to address the errors found by the CI pipeline. If it fails a second time, I'll again update the commit.

aggarg commented 5 months ago

Thank you for raising the PR. We are looking at it and will get back to you.

gemarcano commented 5 months ago

I saw that a unit test failed, something about calling some length function more than expected. Another alternative solution would be to introduce a new macro/function that appends a node to the actual end of the list (say, at the position before xListEnd), and use that instead of vListInsertEnd. That wouldn't require modifying the current loop.

chinglee-iot commented 4 months ago

@gemarcano Thank you for creating this PR. I will look into this PR and the unit test problem. It probably takes some time to look into the detail to discuss with you.

kstribrnAmzn commented 4 months ago

I will defer to @chinglee-iot and @aggarg since they have significantly more experience than I do with the SMP code but I don't personally see an issue with modifying the existing unit test to cover the updated (and correct) code.

aggarg commented 4 months ago

@kstribrnAmzn We are currently evaluating another fix - https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/1000.

sonarcloud[bot] commented 4 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

chinglee-iot commented 4 months ago

1000 to address the same issue is merged. This PR will be closed. Thank you for reporting this issue and creating this PR.