Skip to content

rtic-sync: No need for c-s to check if link is in wait queue if link is popped #1042

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

Merged
merged 3 commits into from
Mar 23, 2025

Conversation

datdenkikniet
Copy link
Contributor

@datdenkikniet datdenkikniet commented Mar 18, 2025

Minor improvement: if we insert our link into the wait queue, we always take a critical section (in the OnDrop) to ensure that it is no longer in the list on drop.

However, if our link is popped, we know for a fact that it is not in the list. take()-ing it in the block that gets us into that state saves us an unnecessary critical section, which is probably the 2nd-most common scenario (the 1st being finding a free slot immediately).

Additionally, we take a critical-section to check whether we still have an entry in our SlotPtr. However, once we are certain that our link is removed from the list, we have exclusive access to it again: hence, we can skip that critical section too, on the happy path.

@datdenkikniet datdenkikniet force-pushed the no-need-for-cs-if-popped branch from 09d7152 to 53fab48 Compare March 18, 2025 21:28
@datdenkikniet datdenkikniet changed the title rtic-sync: No need for cs if wait-queue link is popped rtic-sync: No need for c-s to check if link is in wait queue if link is popped Mar 18, 2025
@datdenkikniet datdenkikniet force-pushed the no-need-for-cs-if-popped branch 3 times, most recently from d41a189 to f612ba0 Compare March 19, 2025 20:47
Copy link
Collaborator

@korken89 korken89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@datdenkikniet
Copy link
Contributor Author

datdenkikniet commented Mar 23, 2025

This will block merging of #1038, so will wait until that is in :)

ETA: I am entirely lying. Will merge this

@datdenkikniet datdenkikniet force-pushed the no-need-for-cs-if-popped branch from f612ba0 to 744d72b Compare March 23, 2025 10:46
@datdenkikniet datdenkikniet added this pull request to the merge queue Mar 23, 2025
Merged via the queue into master with commit d76252d Mar 23, 2025
50 checks passed
@datdenkikniet datdenkikniet deleted the no-need-for-cs-if-popped branch March 23, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants