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

NO-SDV [vioscsi] Reduce spinlock management complexity #1196

Merged

Conversation

benyamin-codez
Copy link
Contributor

Spinlocks are now primarily of the DpcLock type, without which the driver will not properly operate. Fallback to an InterruptLock type spinlock is limited, perhaps even cosmetic.

This commit removes the VioScsiVQLock() and VioScsiVQUnlock() wrapper routines to use the native functions instead, i.e. StorPortAcquireSpinLock() and StorPortReleaseSpinLock(). VioScsiVQLock() contained a superfluous MSI-X and QueueNumber check and VioScsiVQUnlock() did not release non-DpcLock type spinlocks. Removing the wrapper routines also resolves these two issues.

Refactoring includes:

  • Renaming of ProcessQueue() to ProcessBuffer(). Whilst technically more accurate, this also provides space for a new ProcessQueue() routine, via which the HW_MESSAGE_SIGNALLED_INTERRUPT_ROUTINE and HW_INTERRUPT routines can then be consolidated.
  • Where necessary, changing BOOLEAN isr to STOR_SPINLOCK LockMode
  • CamelCase / conformity correction of MessageID to MessageId
  • Conformity correction of QueuNum to QueneNumber
  • Removal of unnecessary use of MsgID and MessageID or MessageId in area of focus (11 instances of MessageID remain in other vioscsi.c routines)
  • Mnemonic rename of:
    • CompletePendingRequests() to CompletePendingRequestsOnReset()
    • index to vq_req_idx
  • Syntactic correction of if statement (added braces)

DUPLICATE of PR #1175 to produce clean rebased version.

Spinlocks are now primarily of the DpcLock type, without which the
driver will not properly operate. Fallback to an InterruptLock type
spinlock is limited, perhaps even cosmetic.

This commit removes the VioScsiVQLock() and VioScsiVQUnlock()
wrapper routines to use the native functions instead, i.e.
StorPortAcquireSpinLock() and StorPortReleaseSpinLock().
VioScsiVQLock() contained a superfluous MSI-X and QueueNumber check
and VioScsiVQUnlock() did not release non-DpcLock type spinlocks.
Removing the wrapper routines also resolves these two issues.

Refactoring includes:
* Renaming of ProcessQueue() to ProcessBuffer(). Whilst technically more
  accurate, this also provides space for a new ProcessQueue() routine,
  via which the HW_MESSAGE_SIGNALLED_INTERRUPT_ROUTINE and HW_INTERRUPT
  routines can then be consolidated.
* Where necessary, changing BOOLEAN isr to STOR_SPINLOCK LockMode
* CamelCase / conformity correction of MessageID to MessageId
* Conformity correction of QueuNum to QueneNumber
* Removal of unnecessary use of MsgID and MessageID or MessageId in area
  of focus (11 instances of MessageID remain in other vioscsi.c routines)
* Mnemonic rename of:
  * CompletePendingRequests() to CompletePendingRequestsOnReset()
  * index to vq_req_idx
* Syntactic correction of if statement (added braces)

DUPLICATE of PR virtio-win#1175 to produce clean rebased version.

Signed-off-by: benyamin-codez <[email protected]>
@YanVugenfirer
Copy link
Collaborator

@benyamin-codez This one looks without merge conflicts

@benyamin-codez
Copy link
Contributor Author

@YanVugenfirer

This one looks without merge conflicts

Yeah... Not sure what was up with #1175, but have closed that one in favour of this clone.

@YanVugenfirer YanVugenfirer merged commit 15e64ac into virtio-win:master Nov 20, 2024
1 of 3 checks passed
@YanVugenfirer
Copy link
Collaborator

@benyamin-codez Thanks a lot!

@MaxXor
Copy link

MaxXor commented Nov 20, 2024

Thanks @benyamin-codez for your work! Much appreciated.

One question for @YanVugenfirer and @vrozenfe : When will you release a new version for public testing?

@benyamin-codez
Copy link
Contributor Author

@MaxXor

I believe b266 was released a couple of days ago:
https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/archive-virtio/virtio-win-0.1.266-1/

I think it should at least have the SendSRB() and DPC fixes, refactoring, tracing improvements, etc.

The tags seem to be missing after mm268/b248...

@vrozenfe
Copy link
Collaborator

I updated tags https://github.com/virtio-win/kvm-guest-drivers-windows/tags and wiki https://github.com/virtio-win/kvm-guest-drivers-windows/wiki/Builds-and-tags-mapping

@vrozenfe
Copy link
Collaborator

Thanks @benyamin-codez for your work! Much appreciated.

One question for @YanVugenfirer and @vrozenfe : When will you release a new version for public testing?

The next stable public build will be released in mid-end of January 2025. But I will try running a new internal build and pushing attestation signed drivers to https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/upstream-virtio/
during the next week.
Vadim.

@benyamin-codez
Copy link
Contributor Author

@vrozenfe Thanks for updating the tags, Vadim.

@MaxXor

I think it should at least have the SendSRB() and DPC fixes, refactoring, tracing improvements, etc.

From the tags it looks like the cut-off was in the ides of October:
https://github.com/virtio-win/kvm-guest-drivers-windows/commits/96b358e3137139d54459107ee8d76baa1a7401d3/

Notably, the viostor back-ported fixes will be missing.
Perhaps a prudent risk mitigation strategy should vioscsi prove to have regressions.

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.

4 participants