Skip to content

Commit

Permalink
NO-SDV [vioscsi] Reduce spinlock management complexity
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@benyamin-codez
benyamin-codez committed Nov 20, 2024
1 parent 7d70e12 commit fe07e50
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 114 deletions.
53 changes: 4 additions & 49 deletions vioscsi/helper.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ SendSRB(
ULONG QueueNumber = VIRTIO_SCSI_REQUEST_QUEUE_0;
BOOLEAN notify = FALSE;
STOR_LOCK_HANDLE LockHandle = { 0 };
PVOID LockContext;
ULONG status = STOR_STATUS_SUCCESS;
UCHAR ScsiStatus = SCSISTAT_GOOD;
ULONG MessageId;
INT add_buffer_req_status = VQ_ADD_BUFFER_SUCCESS;
PREQUEST_LIST element;
ULONG vq_req_idx;
Expand Down Expand Up @@ -94,8 +94,8 @@ ENTER_FN_SRB();
return;
}

MessageId = QUEUE_TO_MESSAGE(QueueNumber);
vq_req_idx = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0;
LockContext = &adaptExt->dpc[vq_req_idx];

if (adaptExt->reset_in_progress) {
RhelDbgPrint(TRACE_LEVEL_FATAL, " Reset is in progress, completing SRB 0x%p with SRB_STATUS_BUS_RESET.\n", Srb);
Expand All @@ -104,7 +104,7 @@ ENTER_FN_SRB();
return;
}

VioScsiVQLock(DeviceExtension, MessageId, &LockHandle, FALSE);
StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
SET_VA_PA();
add_buffer_req_status = virtqueue_add_buf(adaptExt->vq[QueueNumber],
srbExt->psgl,
Expand All @@ -127,7 +127,7 @@ ENTER_FN_SRB();
(add_buffer_req_status == -ENOSPC) ? "ENOSPC" : "UNKNOWN", add_buffer_req_status, QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue);
CompleteRequest(DeviceExtension, Srb);
}
VioScsiVQUnlock(DeviceExtension, MessageId, &LockHandle, FALSE);
StorPortReleaseSpinLock(DeviceExtension, &LockHandle);
if (notify){
virtqueue_notify(adaptExt->vq[QueueNumber]);
}
Expand Down Expand Up @@ -498,51 +498,6 @@ ENTER_FN();
EXIT_FN();
}

VOID
//FORCEINLINE
VioScsiVQLock(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN OUT PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr
)
{
PADAPTER_EXTENSION adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
ULONG QueueNumber = MESSAGE_TO_QUEUE(MessageID);
ENTER_FN();

if (!isr) {
if (adaptExt->msix_enabled) {
// Queue numbers start at 0, message ids at 1.
NT_ASSERT(MessageID > VIRTIO_SCSI_REQUEST_QUEUE_0);
if (QueueNumber >= (adaptExt->num_queues + VIRTIO_SCSI_REQUEST_QUEUE_0)) {
QueueNumber %= adaptExt->num_queues;
}
StorPortAcquireSpinLock(DeviceExtension, DpcLock, &adaptExt->dpc[QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0], LockHandle);
}
else {
StorPortAcquireSpinLock(DeviceExtension, InterruptLock, NULL, LockHandle);
}
}
EXIT_FN();
}

VOID
//FORCEINLINE
VioScsiVQUnlock(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr
)
{
ENTER_FN();
if (!isr) {
StorPortReleaseSpinLock(DeviceExtension, LockHandle);
}
EXIT_FN();
}

VOID FirmwareRequest(
IN PVOID DeviceExtension,
IN PSRB_TYPE Srb
Expand Down
24 changes: 3 additions & 21 deletions vioscsi/helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,28 +171,10 @@ VioScsiCompleteDpcRoutine(
);

VOID
ProcessQueue(
ProcessBuffer(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN BOOLEAN isr
);

VOID
//FORCEINLINE
VioScsiVQLock(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN OUT PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr
);

VOID
//FORCEINLINE
VioScsiVQUnlock(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr
IN ULONG MessageId,
IN STOR_SPINLOCK LockMode
);

VOID
Expand Down
99 changes: 55 additions & 44 deletions vioscsi/vioscsi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ VOID
FORCEINLINE
DispatchQueue(
IN PVOID DeviceExtension,
IN ULONG MessageID
IN ULONG MessageId
);

BOOLEAN
Expand Down Expand Up @@ -1065,7 +1065,7 @@ VioScsiInterrupt(
}
else
{
ProcessQueue(DeviceExtension, QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0), TRUE);
ProcessBuffer(DeviceExtension, QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0), InterruptLock);
}
}

Expand Down Expand Up @@ -1301,16 +1301,16 @@ ENTER_FN();
break;
case ScsiUnitRemove:
case ScsiUnitSurpriseRemoval:
ULONG QueuNum;
ULONG MsgId;
ULONG vq_req_idx;
PREQUEST_LIST element;
STOR_LOCK_HANDLE LockHandle = { 0 };
PVOID LockContext = NULL; //sanity check for LockMode = InterruptLock or StartIoLock
PSTOR_ADDR_BTL8 stor_addr = (PSTOR_ADDR_BTL8)Parameters;

for (index = 0; index < adaptExt->num_queues; index++) {
PREQUEST_LIST element = &adaptExt->processing_srbs[index];
QueuNum = index + VIRTIO_SCSI_REQUEST_QUEUE_0;
MsgId = QUEUE_TO_MESSAGE(QueuNum);
VioScsiVQLock(DeviceExtension, MsgId, &LockHandle, FALSE);
for (vq_req_idx = 0; vq_req_idx < adaptExt->num_queues; vq_req_idx++) {
element = &adaptExt->processing_srbs[vq_req_idx];
LockContext = &adaptExt->dpc[vq_req_idx];
StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
if (!IsListEmpty(&element->srb_list))
{
PLIST_ENTRY entry = element->srb_list.Flink;
Expand All @@ -1331,7 +1331,7 @@ ENTER_FN();
}
}
}
VioScsiVQUnlock(DeviceExtension, MsgId, &LockHandle, FALSE);
StorPortReleaseSpinLock(DeviceExtension, &LockHandle);
}
Status = ScsiUnitControlSuccess;
break;
Expand Down Expand Up @@ -1462,7 +1462,7 @@ VOID
FORCEINLINE
DispatchQueue(
IN PVOID DeviceExtension,
IN ULONG MessageID
IN ULONG MessageId
)
{
PADAPTER_EXTENSION adaptExt;
Expand All @@ -1471,42 +1471,52 @@ ENTER_FN();
adaptExt = (PADAPTER_EXTENSION)DeviceExtension;

if (!adaptExt->dump_mode && adaptExt->dpc_ok) {
NT_ASSERT(MessageID >= QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0));
NT_ASSERT(MessageId >= QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0));
StorPortIssueDpc(DeviceExtension,
&adaptExt->dpc[MessageID - QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0)],
ULongToPtr(MessageID),
ULongToPtr(MessageID));
&adaptExt->dpc[MessageId - QUEUE_TO_MESSAGE(VIRTIO_SCSI_REQUEST_QUEUE_0)],
ULongToPtr(MessageId),
ULongToPtr(MessageId));
EXIT_FN();
return;
}
ProcessQueue(DeviceExtension, MessageID, TRUE);
ProcessBuffer(DeviceExtension, MessageId, InterruptLock);
EXIT_FN();
}

VOID
ProcessQueue(
ProcessBuffer(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN BOOLEAN isr
IN ULONG MessageId,
IN STOR_SPINLOCK LockMode
)
{
PVirtIOSCSICmd cmd;
unsigned int len;
PADAPTER_EXTENSION adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
ULONG index = MESSAGE_TO_QUEUE(MessageID);
STOR_LOCK_HANDLE queueLock = { 0 };
ULONG QueueNumber = MESSAGE_TO_QUEUE(MessageId);
STOR_LOCK_HANDLE LockHandle = { 0 };
struct virtqueue *vq;
PSRB_TYPE Srb = NULL;
PSRB_EXTENSION srbExt = NULL;
if (index >= (adaptExt->num_queues + VIRTIO_SCSI_REQUEST_QUEUE_0)) {
index %= adaptExt->num_queues;
}
PREQUEST_LIST element = &adaptExt->processing_srbs[index - VIRTIO_SCSI_REQUEST_QUEUE_0];
PREQUEST_LIST element;
ULONG vq_req_idx;
PVOID LockContext = NULL; //sanity check for LockMode = InterruptLock or StartIoLock

ENTER_FN();
vq = adaptExt->vq[index];

VioScsiVQLock(DeviceExtension, MessageID, &queueLock, isr);
if (QueueNumber >= (adaptExt->num_queues + VIRTIO_SCSI_REQUEST_QUEUE_0)) {
RhelDbgPrint(TRACE_LEVEL_VERBOSE, " Modulo assignment required for QueueNumber as it exceeds the number of virtqueues available.\n");
QueueNumber %= adaptExt->num_queues;
}
vq_req_idx = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0;
element = &adaptExt->processing_srbs[vq_req_idx];

vq = adaptExt->vq[QueueNumber];

if (LockMode == DpcLock) {
LockContext = &adaptExt->dpc[vq_req_idx];
}
StorPortAcquireSpinLock(DeviceExtension, LockMode, LockContext, &LockHandle);

do {
virtqueue_disable_cb(vq);
Expand All @@ -1515,9 +1525,9 @@ ENTER_FN();
BOOLEAN bFound = FALSE;

Srb = (PSRB_TYPE)(cmd->srb);
if (!Srb)
if (!Srb) {
continue;

}
srbExt = SRB_EXTENSION(Srb);
for (le = element->srb_list.Flink; le != &element->srb_list && !bFound; le = le->Flink)
{
Expand All @@ -1538,7 +1548,7 @@ ENTER_FN();
}
} while (!virtqueue_enable_cb(vq));

VioScsiVQUnlock(DeviceExtension, MessageID, &queueLock, isr);
StorPortReleaseSpinLock(DeviceExtension, &LockHandle);

EXIT_FN();
}
Expand All @@ -1555,18 +1565,21 @@ VioScsiCompleteDpcRoutine(

ENTER_FN();
MessageId = PtrToUlong(SystemArgument1);
ProcessQueue(Context, MessageId, FALSE);
ProcessBuffer(Context, MessageId, DpcLock);
EXIT_FN();
}

VOID
CompletePendingRequests(
CompletePendingRequestsOnReset(
IN PVOID DeviceExtension
)
{
PADAPTER_EXTENSION adaptExt;
ULONG QueueNum;
ULONG MsgId;
PADAPTER_EXTENSION adaptExt;
ULONG QueueNumber;
ULONG vq_req_idx;
PREQUEST_LIST element;
STOR_LOCK_HANDLE LockHandle = { 0 };
PVOID LockContext = NULL; //sanity check for LockMode = InterruptLock or StartIoLock

adaptExt = (PADAPTER_EXTENSION)DeviceExtension;

Expand All @@ -1576,13 +1589,11 @@ CompletePendingRequests(
StorPortPause(DeviceExtension, 10);
DeviceReset(DeviceExtension);

for (ULONG index = 0; index < adaptExt->num_queues; index++) {
PREQUEST_LIST element = &adaptExt->processing_srbs[index];
STOR_LOCK_HANDLE LockHandle = { 0 };
RhelDbgPrint(TRACE_LEVEL_FATAL, " queue %d cnt %d\n", index, element->srb_cnt);
QueueNum = index + VIRTIO_SCSI_REQUEST_QUEUE_0;
MsgId = QUEUE_TO_MESSAGE(QueueNum);
VioScsiVQLock(DeviceExtension, MsgId, &LockHandle, FALSE);
for (vq_req_idx = 0; vq_req_idx < adaptExt->num_queues; vq_req_idx++) {
element = &adaptExt->processing_srbs[vq_req_idx];
RhelDbgPrint(TRACE_LEVEL_FATAL, " queue %d cnt %d\n", vq_req_idx, element->srb_cnt);
LockContext = &adaptExt->dpc[vq_req_idx];
StorPortAcquireSpinLock(DeviceExtension, DpcLock, LockContext, &LockHandle);
while (!IsListEmpty(&element->srb_list)) {
PLIST_ENTRY entry = RemoveHeadList(&element->srb_list);
if (entry) {
Expand All @@ -1598,7 +1609,7 @@ CompletePendingRequests(
if (element->srb_cnt) {
element->srb_cnt = 0;
}
VioScsiVQUnlock(DeviceExtension, MsgId, &LockHandle, FALSE);
StorPortReleaseSpinLock(DeviceExtension, &LockHandle);
}
StorPortResume(DeviceExtension);
}
Expand Down Expand Up @@ -1685,7 +1696,7 @@ ENTER_FN_SRB();
switch (adaptExt->action_on_reset) {
case VioscsiResetCompleteRequests:
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " Completing all pending SRBs\n");
CompletePendingRequests(DeviceExtension);
CompletePendingRequestsOnReset(DeviceExtension);
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_SUCCESS);
return TRUE;
case VioscsiResetDoNothing:
Expand Down

0 comments on commit fe07e50

Please sign in to comment.