nvme_test: update shadow doorbell handling to mirror nvme crate #2007
Conversation
Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com>
Copilot
AI
changed the title
| shadow_db_evt_idx: Option<ShadowDoorbell>, | ||
| ) -> Self { | ||
| tail.write(0); | ||
| pub fn new(cq: &CompletionQueue, db_id: u16, gpa: u64, len: u16) -> Self { |
There was a problem hiding this comment.
@copilot The update to the submission queue definition doesn't actually happen until after the commit I asked you to apply here. Can you please fix that? The SubmissionQueue::new function should look like impl SubmissionQueue {
pub fn new(
doorbells: Arc<RwLock>,
db_id: u16,
mem: GuestMemory,
gpa: u64,
len: u16,
) -> Self {
doorbells.read().write(db_id, 0);
Self {
tail: DoorbellState::new(doorbells, db_id, len.into()),
head: 0,
gpa,
mem,
}
}
Can you fix this and any associated areas where you are creating SubmissionQueues as well
There was a problem hiding this comment.
Fixed the SubmissionQueue::new function signature to take doorbells: Arc<RwLock<DoorbellMemory>> as a direct parameter instead of extracting it from the completion queue. Updated call sites in admin.rs and io.rs accordingly. The change is in commit c729103.
Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com>
| }) | ||
| .collect(); | ||
|
|
||
| let admin_cq = CompletionQueue::new( |
There was a problem hiding this comment.
@copilot for the sake of consistency can you please move this CompletionQueue::new() in to the Self{} creation below?
There was a problem hiding this comment.
Moved CompletionQueue::new() inside the Self{} creation for consistency as requested. The change is in commit 4d8ec6f.
Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com>
|
@copilot one last thing, can you also update the shadow doorbell tests in the nvme_test crate to match the changes from that commit? |
Co-authored-by: gurasinghMS <127339643+gurasinghMS@users.noreply.github.com>
Updated the shadow doorbell tests in the nvme_test crate to match the new DoorbellMemory API. Removed the deprecated |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the nvme_test crate to mirror the shadow doorbell handling improvements implemented in the main nvme crate. The changes replace individual DoorbellRegister instances with a unified DoorbellMemory system and simplify event index management by making shadow doorbell memory the authoritative source for doorbell values.
Key Changes
- Introduced
DoorbellMemoryandDoorbellStatefor unified doorbell management - Simplified event index logic by updating only when polling for work on empty queues
- Replaced complex shadow doorbell heuristics with polling-based async queue handling
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/queue.rs |
Complete rewrite implementing new DoorbellMemory and DoorbellState architecture |
src/workers/io.rs |
Updated to use new queue interfaces and removed manual event index management |
src/workers/admin.rs |
Updated queue initialization and doorbell buffer config handling |
src/workers/coordinator.rs |
Updated to use unified doorbell memory management |
src/pci.rs |
Updated doorbell write handling with new variable names |
src/tests/shadow_doorbell_tests.rs |
Updated tests to use new DoorbellMemory API |
Cargo.toml |
Removed unused event-listener dependency |
| spec::AdminOpcode::DOORBELL_BUFFER_CONFIG | ||
| if self.supports_shadow_doorbells(state) => | ||
| { | ||
| self.handle_doorbell_buffer_config(state, &command) | ||
| .await | ||
| .map(|()| Some(Default::default())) | ||
| } |
There was a problem hiding this comment.
[nitpick] The guard condition if self.supports_shadow_doorbells(state) is misaligned with the match arm. Consider reformatting for better readability or extracting this logic to a separate condition before the match statement.
Copilot uses AI. Check for mistakes.
4 participants
This PR updates the
nvme_testcrate to incorporate the shadow doorbell improvements that were implemented in the mainnvmecrate in commit 94889ca. The changes ensure consistency between the two implementations and provide better shadow doorbell support in the test crate.Key Changes
New Queue Architecture
DoorbellRegisterinstances with a unifiedDoorbellMemorysystemDoorbellStatefor polling-based async queue handlingDoorbellMemoryinstanceSimplified Event Index Logic
Shadow Doorbell as Single Source of Truth
Files Updated
SubmissionQueue::newsignatureBenefits
Latest Changes
SubmissionQueue::newsignature to takedoorbellsparameter directlyCompletionQueue::new()insideSelf{}creation for consistencyShadow Doorbell Test Updates
ShadowDoorbellstruct usage from testsDOORBELL_BUFFER_BASEandEVT_IDX_BUFFER_BASEILLEGAL_DOORBELL_VALUEconstant usageThe changes maintain full backward compatibility while providing the enhanced shadow doorbell support that matches the production
nvmecrate implementation.Fixes #2006.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.