oxidecomputer / oxidecomputer/propolis
nvme: set completion queue EventIdx more carefully
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 270
- Forks
- 42
- Avg merge
- 4d 5h
- Merged PRs (30d)
- 6
Description
https://github.com/oxidecomputer/propolis/pull/1026 took a big hammer to it by configuring EventIdx such that we always get a doorbell when a CQ is emptied, but we really only need to hold EventIdx back if we might have gotten an SQ corked on the CQ. then, the doorbell is load-bearing to tell us to wake the SQ and finally actually do the I/O.
I think we want to only let EventIdx go so far forward that the CQ head<->CQ EventIdx size is the CQ size minus the number of SQs that complete into that queue. there is some raciness here I haven't convinced myself about though: if a CQ has one SQ pointed at it, CQ_EventIdx-CQ_head == 1, then the guest configures a second SQ to that first CQ, then both submission queues get I/Os, one of the SQs could end up corked while the other takes the remaining CQ space.
that might be fine if when we read the CQ shadow doorbell and discover the CQ is no longer full, we kick min(CQ_space, corked_SQs) so that I/Os get processed.
otoh, by setting EventIdx to one before the CQ tail, we might have to wait for the guest to get through a deep (Linux picks 1024) queue of completions before it notifies us there's space again. at 200 nanoseconds per completion (number is fake and invented) that's 200 microseconds of latency to resume I/Os if the queue is full. we might want to limit EventIdx to be at most 64 away from the CQ tail, which also avoids the SQs-stay-corked-forever issue above. then, CQ doorbells that we didn't need are an indicator that we're not pushing EventIdx far enough out, and maybe we should let it go higher?
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading pull request #1026 and the NVMe completion- and submission-queue handling it changed. Work out the EventIdx invariant, the CQ shadow-doorbell and corked-SQ race, and the latency tradeoff before choosing an approach. Done means the policy is agreed and its full, shared, and deep-queue cases are validated.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- operating-systems, performance
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100