Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix prvSelectHighestPriorityTask ready list iteration #991

Closed

Conversation

gemarcano
Copy link

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:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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 gemarcano requested a review from a team as a code owner February 13, 2024 19:53
@gemarcano
Copy link
Author

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
Copy link
Member

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

@gemarcano gemarcano force-pushed the fix_scheduler_ready_list_iteration branch from af3b9ed to e17b7b5 Compare February 14, 2024 05:22
@gemarcano
Copy link
Author

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
Copy link
Member

aggarg commented Feb 14, 2024

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

 - Use listGET_OWNER_OF_NEXT_ENTRY to iterate through the ready task
   list instead of iterating from the head of the list. vListInsertEnd
   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 tail of the
   list.
@gemarcano gemarcano force-pushed the fix_scheduler_ready_list_iteration branch from e17b7b5 to 9d59571 Compare February 14, 2024 17:13
@gemarcano
Copy link
Author

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.

gemarcano added a commit to gemarcano/snes_controllers_to_usb that referenced this pull request Feb 15, 2024
 - Software is based off my wifi pc remote button code, as well as my
   old SNES STM32 controller converter code for the TinyUSB descriptor.
 - This code relies on the following upstream PRs:
   - raspberrypi/pico-sdk#1530
   - FreeRTOS/FreeRTOS-Kernel#991
   - raspberrypi/pico-sdk#1635
@chinglee-iot
Copy link
Member

@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.

@chinglee-iot chinglee-iot self-assigned this Feb 19, 2024
@kstribrnAmzn
Copy link
Member

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
Copy link
Member

aggarg commented Feb 26, 2024

@kstribrnAmzn We are currently evaluating another fix - #1000.

Copy link

sonarcloud bot commented Mar 4, 2024

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
Copy link
Member

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

laroche pushed a commit to laroche/FreeRTOS-Kernel that referenced this pull request Apr 18, 2024
* Add Reg tests for CORTEX M33F Keil Simulator Project

* Code review suggestions

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>

* Fix hard fault because of main stack overflow

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>

---------

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
Co-authored-by: Gaurav Aggarwal <aggarg@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants