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

feat(threads): sort ThreadList by priority #381

Merged

Conversation

elenaf9
Copy link
Collaborator

@elenaf9 elenaf9 commented Aug 14, 2024

Description

Insert new threads in ThreadList based on priorities, so that the highest priority thread is the head.
Within a priority the threads are ordered in FIFO.

This reduces priority inversions, and will facilitate proper priority inheritance in the future.

The drawback of this change is that inserting a new Id in the ThreadList is now more complex. Not sure if we can avoid this in any way.

Issues/PRs references

Alternative to #371, which implements the ThreadList as pure FIFO.

Open Questions

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@kaspar030
Copy link
Collaborator

Nice! Changes to riot-rs-threads look fine.

Can we turn the example into a test (in tests/), and add some logic after each acquire() that e.g., checks the value of an atomic uint if it contains the right value, so we can test the order programatically? (e.g., first thread expects a zero, then sets bit zero. second expects bit zero, then ORs in bit one. third one expects bits zero and one, ...)

If you feel changing that now is out of scope, I'd suggest dropping the example and we add tests later.

@elenaf9 elenaf9 force-pushed the threads/prio-sorted-blocklist branch 2 times, most recently from 149bf37 to 67a8e9c Compare September 9, 2024 08:31
@elenaf9
Copy link
Collaborator Author

elenaf9 commented Sep 9, 2024

Can we turn the example into a test (in tests/), and add some logic after each acquire() that e.g., checks the value of an atomic uint if it contains the right value, so we can test the order programatically? (e.g., first thread expects a zero, then sets bit zero. second expects bit zero, then ORs in bit one. third one expects bits zero and one, ...)

Wdyt of 67a8e9c?

@elenaf9 elenaf9 marked this pull request as ready for review September 9, 2024 08:32
@kaspar030
Copy link
Collaborator

LGTM, please squash!

Insert new threads in `ThreadList` based on priorities, so that the
highest priority thread is the head.
Within a prio the threads are ordered in FIFO.
This demonstrates how multiple threads can wait for the same lock and
get unblocked by priority, and within a prio by FIFO order.
@kaspar030 kaspar030 added this pull request to the merge queue Sep 13, 2024
Merged via the queue into future-proof-iot:main with commit 69853b5 Sep 13, 2024
26 checks passed
@elenaf9 elenaf9 deleted the threads/prio-sorted-blocklist branch September 13, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants