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

Remove ackable_work; ack immediately instead #1552

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Nov 5, 2024

A long time ago, one task accumulated ackable jobs into Downstairs::ackable_work and a different task periodically read out that set and sent acks back to the guest.

The tasks have long since been consolidated, but the ackable_work set remained.

This PR removes the ackable_work set, acking jobs immediately when they are ready. It's a significant LOC simplification, because it's removing intermediate state (which previously had to be checked in unit tests).

There's one subtlety in handling live-repair: previously, the live-repair check would look in ackable_work to see if the next job was ackable, which indicates that live-repair can continue. Now, we update the LiveRepairState directly in Downstairs::ack, storing a result for the pending live-repair job.

Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

We had a separate task (way way back) in part to make progress if queues were full and we needed to make forward progress. I don't believe that situation still exists.

@mkeeter mkeeter force-pushed the mkeeter/immediate-ack branch 2 times, most recently from 25abe34 to 3329865 Compare November 11, 2024 21:38
upstairs/src/downstairs.rs Outdated Show resolved Hide resolved
@mkeeter mkeeter merged commit 515725c into main Nov 15, 2024
6 checks passed
@mkeeter mkeeter deleted the mkeeter/immediate-ack branch November 15, 2024 16:20
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.

3 participants