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.

Sign up for GitHub

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

[vioscsi] Fix SendSRB regression and refactor for optimum performance [viostor] Backport SendSRB improvements #1135

Closed
wants to merge 5 commits into from
Closed

[vioscsi] Fix SendSRB regression and refactor for optimum performance [viostor] Backport SendSRB improvements #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a719ccd
[vioscsi] Fixed regression in SendSRB and refactored to improve perfo…
invalid-email-address Aug 16, 2024
4d4bf72
Merge branch 'virtio-win:master' into SendSRB-fix
benyamin-codez Aug 20, 2024
2887e8b
[viostor] Backported SendSRB improvements from vioscsi
invalid-email-address Aug 20, 2024
2e033cb
Merge branch 'SendSRB-fix' of github.com:benyamin-codez/kvm-guest-dri…
invalid-email-address Aug 20, 2024
976c325
[vioscsi] Updated WPP and refactored for optimum performance
invalid-email-address Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 122 additions & 83 deletions vioscsi/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,25 @@ SendSRB(
UCHAR ScsiStatus = SCSISTAT_GOOD;
ULONG MessageID;
int res = 0;
BOOLEAN notify = FALSE;
PREQUEST_LIST element;
ULONG index;
ENTER_FN_SRB();

if (!Srb)
return;
ENTER_FN_SRB();

if (!Srb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, do we ever hit this scenario where we are coming into SendSRB with a null Srb? Meaning, are there any callers that actually can do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seemed like a sanity check to me. It lives on, so maybe someone saw one in the past. We can add a WPP macro here to check and use a reserved flag and just look for that in the trace. It would take pretty extreme performance testing, and more to the point - fairly thorough fail condition testing. Maybe raise a new issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, it is a kind of sanity check. Static driver verifier was failing without this check.

EXIT_FN_SRB();
Copy link
Contributor

Choose a reason for hiding this comment

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

EXIT_FN_SRB() is an alias for debuggin, right? if so, can we align/indent appropriately here? Means it easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question here I think is: does it stay or does it go...? Another call slows the driver down.
Maybe this is technically a macro, so normally they wouldn't be indented methinks.
@vrozenfe can probably answer this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably worth mentioning that we can probably add some conditional blocks like #ifndef TRACE_VQLOCKS or #ifndef TRACE_SRBS to wrap these. Then indenting might be less of an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other option here, if everyone is ok with using it would be to use goto label_at_end_of_function; and then drop label_at_end_of_function: just before the WPP macro call. I'm fine with that. It's really what goto in C is supposed to be used for and cannot be used to jump out of the function like in other langs. I see it in [virtio] in some places. Many projects frown on using goto though, so...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... goto is also faster - which I'm guessing we all like...

Copy link
Contributor Author

@benyamin-codez benyamin-codez Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively, we can just drop an appropriately indented RhelDbgPrint(); and leave it at that.
If we use say RhelDbgPrint(TRACE_LEVEL_RESERVED8, " After KickPrep Notify State : %d \n", notify); that will also work with WPP provided TRACE_LEVEL_RESERVED8 is defined in WPP_DEFINE_CONTROL_GUID(). You can then just toggle that flag to enable or disable the trace.

Any of these work for me. What I think is most important is that there is working tracing...

return;
}

if (adaptExt->bRemoved) {
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_NO_DEVICE);
CompleteRequest(DeviceExtension, Srb);
return;
}

EXIT_FN_SRB();
benyamin-codez marked this conversation as resolved.
Show resolved Hide resolved
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this extra indent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, nor the indent for the brace at L75.

}
LOG_SRB_INFO();

if (adaptExt->num_queues > 1) {
STARTIO_PERFORMANCE_PARAMETERS param;
param.Size = sizeof(STARTIO_PERFORMANCE_PARAMETERS);
Expand All @@ -82,53 +86,55 @@ ENTER_FN_SRB();
}
} else {
RhelDbgPrint(TRACE_LEVEL_ERROR, " StorPortGetStartIoPerfParams failed srb 0x%p status 0x%x MessageNumber %d.\n", Srb, status, param.MessageNumber);
// FIXME
// Should we return on this error..?
// Should it be TRACE_LEVEL_FATAL..?
//is_done = TRUE;
}
}

srbExt = SRB_EXTENSION(Srb);

if (!srbExt) {
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " No SRB Extenstion for SRB 0x%p \n", Srb);
EXIT_FN_SRB();
benyamin-codez marked this conversation as resolved.
Show resolved Hide resolved
return;
}

MessageID = QUEUE_TO_MESSAGE(QueueNumber);
index = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0;

if (adaptExt->reset_in_progress) {
RhelDbgPrint(TRACE_LEVEL_FATAL, " Reset is in progress, completing SRB 0x%p with SRB_STATUS_BUS_RESET.\n", Srb);
RhelDbgPrint(TRACE_LEVEL_WARNING, " Reset is in progress, completing SRB 0x%p with SRB_STATUS_BUS_RESET.\n", Srb);
Copy link
Contributor

Choose a reason for hiding this comment

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

no problem, but why go from FATAL to WARNING here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm presuming that we have historically always wanted to see this. In WPP, I think this would be TRACE_LEVEL_NONE, but in Dbg this probably wouldn't work. However, it is actually just a warning, and should succeed following the reset. If not, this should generate an TRACE_LEVEL_ERROR or TRACE_LEVEL_FATAL event.

SRB_SET_SRB_STATUS(Srb, SRB_STATUS_BUS_RESET);
CompleteRequest(DeviceExtension, Srb);
EXIT_FN_SRB();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, there's going to be lots... 8^d

return;
}

VioScsiVQLock(DeviceExtension, MessageID, &LockHandle, FALSE);
VioScsiLockManager(DeviceExtension, MessageID, &LockHandle, FALSE, VIOSCSI_VQLOCKOP_LOCK);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO make the the introduction and cut-in of VioScsiLockManager a completely separate commit/PR. Then have another commit/PR that does the move-around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the technology. <-- I hope you get the reference...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, the idea here was to permit WPP tracing of each lock/unlock function whilst optimising. The problem with optimising and WPP is that [%!FUNC!] becomes the parent function instead. So it makes sense to me that this would form part of the WPP PR. That being said, I can certainly do a separate commit and PR for it.

SET_VA_PA();
res = virtqueue_add_buf(adaptExt->vq[QueueNumber],
srbExt->psgl,
srbExt->out, srbExt->in,
&srbExt->cmd, va, pa);

if (res >= 0) {
notify = virtqueue_kick_prepare(adaptExt->vq[QueueNumber]);
Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, in this case, we're only //potentially// kicking the underlying device if we're successful in enqueueing work.

This function also has a membarrier, which are are now doing under the lock, vs outside the lock in the previous code

I certainly love it when stuff is faster, but it isn't immediately clearer to me why this code would be more performant than the existing code. Is the idea that we're changing the amount of kicks happening for the better? Would love to know your observations here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the breaking change following the v208 package.
This is where I'm now focusing my efforts per update in #756...
The underlying issue does certainly seem to be about managing virtqueue notifications...

element = &adaptExt->processing_srbs[index];
InsertTailList(&element->srb_list, &srbExt->list_entry);
element->srb_cnt++;
}
VioScsiVQUnlock(DeviceExtension, MessageID, &LockHandle, FALSE);
if ( res >= 0){
if (virtqueue_kick_prepare(adaptExt->vq[QueueNumber])) {
virtqueue_notify(adaptExt->vq[QueueNumber]);
}
} else {
virtqueue_notify(adaptExt->vq[QueueNumber]);
ScsiStatus = SCSISTAT_QUEUE_FULL;
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_BUSY);
SRB_SET_SCSI_STATUS(Srb, ScsiStatus);
StorPortBusy(DeviceExtension, 10);
CompleteRequest(DeviceExtension, Srb);
RhelDbgPrint(TRACE_LEVEL_FATAL, " Could not put an SRB into a VQ, so complete it with SRB_STATUS_BUSY. QueueNumber = %d, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue);
RhelDbgPrint(TRACE_LEVEL_WARNING, " Could not put an SRB into a VQ, so complete it with SRB_STATUS_BUSY. QueueNumber = %d, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue);
}
VioScsiLockManager(DeviceExtension, MessageID, &LockHandle, FALSE, VIOSCSI_VQLOCKOP_UNLOCK);
Copy link
Contributor

Choose a reason for hiding this comment

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

So is the bug here that we're simply unlocking the queue too soon, such that when there is the error path here, things can get jumbled up and blow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so, but probably something underlying in the virtqueue implementation too.

Copy link

Choose a reason for hiding this comment

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

I think this change doesn't just touch the error path but also the happy path (res >= 0):

  • Without this change (on 54442f0), a possible sequence of events for the happy path is 
    VioScsiVQLock -> virtqueue_add_buf -> VioScsiVQUnlock -> virtqueue_kick_prepare -> virtqueue_notify
    so virtqueue_kick_prepare and virtqueue_notify happen outside the lock.
  • With this change, the sequence of events is 
    VioScsiVQLock -> virtqueue_add_buf -> virtqueue_kick_prepare -> VioScsiVQUnlock ->  virtqueue_notify
    So with this change, virtqueue_kick_prepare happens inside the lock, virtqueue_notify happens outside the lock.

I'm no VirtIO expert so I can't really say whether virtqueue_kick_prepare is supposed to happen inside or outside the lock. But FWIW, to me it looks like the Linux virtio scsi driver also kicks inside the lock and notifies outside the lock:

https://github.com/torvalds/linux/blob/b311c1b497e51a628aa89e7cb954481e5f9dced2/drivers/scsi/virtio_scsi.c#L484

Also, it looks like until and including 7dc052d, the virtqueue_kick_prepare used to happen inside the lock:

kvm-guest-drivers-windows/vioscsi/helper.c

Line 112 in 7dc052d

notify = virtqueue_kick_prepare(adaptExt->vq[QueueNumber]) ? TRUE : notify;

and happens outside the lock starting with f1338bb (this commit is also mentioned by @benyamin-codez)

kvm-guest-drivers-windows/vioscsi/helper.c

Line 109 in f1338bb

if (virtqueue_kick_prepare(adaptExt->vq[QueueNumber])) {

Could the early unlock be the cause for the vioscsi "Reset to device \Device\RaidPortN" + IO lockups?

Copy link
Contributor Author

@benyamin-codez benyamin-codez Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could the early unlock be the cause for the vioscsi "Reset to device \Device\RaidPortN" + IO lockups?

Yes, under high load conditions, this could certainly be a trigger.

It's probably worth pointing out that virtqueue_kick_prepare() and virtqueue_kick() are two different things. An actual kick includes the virtqueue_notify(), which executes the notification callback on the VQ.

Here is the latter from VirtIOPCICommon.c:

void virtqueue_notify(struct virtqueue *vq)
{
    vq->notification_cb(vq);
}

void virtqueue_kick(struct virtqueue *vq)
{
    if (virtqueue_kick_prepare(vq)) {
        virtqueue_notify(vq);
    }
}

However, virtqueue_kick_prepare() - which in our case calls virtqueue_kick_prepare_split() - prepares the buffer and returns TRUE if the device needs to be notified. Here is the return comparator for reference:

if (_vq->vdev->event_suppression_enabled) {
    return wrap_around || (bool)vring_need_event(vring_avail_event(&vq->vring), new, old);
} else {
    return !(vq->vring.used->flags & VIRTQ_USED_F_NO_NOTIFY);
}

...and:

/* The Host uses this in used->flags to advise the Guest: don't kick me when
 * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest
 * will still kick if it's out of buffers.  */
#define VIRTQ_USED_F_NO_NOTIFY	1

It also bears mentioning, that even when a notification callback is required it is completely up to the device as to what it will do. It may well just ignore the kicks...

if (notify){
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we not want to kick the underlying backend in the failure case?

In the QUEUE_FULL branch above, notify will be false, yea? In which case, we wont send a kick. I'd imagine we would want to kick in unilaterally, just in case it needs to be hit with a hammer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think there was a notify = TRUE; at the foot of the QUEUE_FULL branch previously. I didn't notice any change with or without it in testing. I can have another go around though, but it could be in virtqueue implementation (which I'm presently looking at).

Copy link
Contributor Author

@benyamin-codez benyamin-codez Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So it appears that res will only ever (famous last words) be a 0 (SUCCESS) or a 28 (ENOSPC: No space left on device), i.e. SCSISTAT_QUEUE_FULL. The test should really be if (res == 0) {}...

As no data was added to a buffer, does it really need any further action?

You might observe in my reply to @frwbr above that a call to virtqueue_kick() just calls virtqueue_kick_prepare() and so a virtqueue_notify(vq) won't happen without the former returning 0.

We could call a virtqueue_notify(vq), but the device won't find anything there.

My thoughts were that it would just consume compute resources and time.

virtqueue_notify(adaptExt->vq[QueueNumber]);
}

EXIT_FN_SRB();
}

Expand Down Expand Up @@ -185,6 +191,7 @@ DeviceReset(

ENTER_FN();
if (adaptExt->dump_mode) {
EXIT_FN();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto on my comments about indentation if we can get away with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let's wait to see what Vadim says.

return TRUE;
}
ASSERT(adaptExt->tmf_infly == FALSE);
Expand All @@ -197,7 +204,7 @@ ENTER_FN();
cmd->req.tmf.lun[3] = 0;
cmd->req.tmf.type = VIRTIO_SCSI_T_TMF;
cmd->req.tmf.subtype = VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET;

srbExt->psgl = srbExt->vio_sg;
srbExt->pdesc = srbExt->desc_alias;
sgElement = 0;
Expand All @@ -212,9 +219,11 @@ ENTER_FN();
StorPortPause(DeviceExtension, 60);
if (!SendTMF(DeviceExtension, Srb)) {
StorPortResume(DeviceExtension);
EXIT_FN();
return FALSE;
}
adaptExt->tmf_infly = TRUE;
EXIT_FN();
return TRUE;
}

Expand Down Expand Up @@ -384,46 +393,48 @@ ENTER_FN();
SP_INTERNAL_ADAPTER_ERROR,
__LINE__);
RhelDbgPrint(TRACE_LEVEL_FATAL, " CANNOT READ PCI CONFIGURATION SPACE %d\n", pci_cfg_len);
EXIT_FN();
Copy link
Contributor

Choose a reason for hiding this comment

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

this whole part should be a separate PR. Not that its bad, just seems orthogonal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was that in the trace I could see entry into the functions, but when leaving early, it indicated a potential problem, because the corresponding exit trace was missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jon, if you were actually talking about the section below, it was because it was in orphaned braces.

return FALSE;
}


UCHAR CapOffset;
PPCI_MSIX_CAPABILITY pMsixCapOffset;
PPCI_COMMON_HEADER pPciComHeader;
pPciComHeader = &adaptExt->pci_config;
if ((pPciComHeader->Status & PCI_STATUS_CAPABILITIES_LIST) == 0)
{
UCHAR CapOffset;
PPCI_MSIX_CAPABILITY pMsixCapOffset;
PPCI_COMMON_HEADER pPciComHeader;
pPciComHeader = &adaptExt->pci_config;
if ((pPciComHeader->Status & PCI_STATUS_CAPABILITIES_LIST) == 0)
{
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " NO CAPABILITIES_LIST\n");
}
else
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " NO CAPABILITIES_LIST\n");
} else
{
if ((pPciComHeader->HeaderType & (~PCI_MULTIFUNCTION)) == PCI_DEVICE_TYPE)
{
if ((pPciComHeader->HeaderType & (~PCI_MULTIFUNCTION)) == PCI_DEVICE_TYPE)
CapOffset = pPciComHeader->u.type0.CapabilitiesPtr;
while (CapOffset != 0)
{
CapOffset = pPciComHeader->u.type0.CapabilitiesPtr;
while (CapOffset != 0)
pMsixCapOffset = (PPCI_MSIX_CAPABILITY)&adaptExt->pci_config_buf[CapOffset];
if (pMsixCapOffset->Header.CapabilityID == PCI_CAPABILITY_ID_MSIX)
{
pMsixCapOffset = (PPCI_MSIX_CAPABILITY)&adaptExt->pci_config_buf[CapOffset];
if (pMsixCapOffset->Header.CapabilityID == PCI_CAPABILITY_ID_MSIX)
{
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.TableSize = %d\n", pMsixCapOffset->MessageControl.TableSize);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.FunctionMask = %d\n", pMsixCapOffset->MessageControl.FunctionMask);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.MSIXEnable = %d\n", pMsixCapOffset->MessageControl.MSIXEnable);

RhelDbgPrint(TRACE_LEVEL_INFORMATION, " MessageTable = %lu\n", pMsixCapOffset->MessageTable.TableOffset);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " PBATable = %lu\n", pMsixCapOffset->PBATable.TableOffset);
adaptExt->msix_enabled = (pMsixCapOffset->MessageControl.MSIXEnable == 1);
} else
{
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " CapabilityID = %x, Next CapOffset = %x\n", pMsixCapOffset->Header.CapabilityID, CapOffset);
}
CapOffset = pMsixCapOffset->Header.Next;
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.TableSize = %d\n", pMsixCapOffset->MessageControl.TableSize);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.FunctionMask = %d\n", pMsixCapOffset->MessageControl.FunctionMask);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, "MessageControl.MSIXEnable = %d\n", pMsixCapOffset->MessageControl.MSIXEnable);

RhelDbgPrint(TRACE_LEVEL_INFORMATION, " MessageTable = %lu\n", pMsixCapOffset->MessageTable.TableOffset);
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " PBATable = %lu\n", pMsixCapOffset->PBATable.TableOffset);
adaptExt->msix_enabled = (pMsixCapOffset->MessageControl.MSIXEnable == 1);
} else
{
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " CapabilityID = %x, Next CapOffset = %x\n", pMsixCapOffset->Header.CapabilityID, CapOffset);
}
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " msix_enabled = %d\n", adaptExt->msix_enabled);
} else
{
RhelDbgPrint(TRACE_LEVEL_FATAL, " NOT A PCI_DEVICE_TYPE\n");
CapOffset = pMsixCapOffset->Header.Next;
}
RhelDbgPrint(TRACE_LEVEL_INFORMATION, " msix_enabled = %d\n", adaptExt->msix_enabled);
} else
{
RhelDbgPrint(TRACE_LEVEL_FATAL, " NOT A PCI_DEVICE_TYPE\n");
// FIXME
// Should we not return on this error..?
//EXIT_FN();
//return FALSE;
}
}

Expand All @@ -435,19 +446,21 @@ ENTER_FN();
if (iBar == -1) {
RhelDbgPrint(TRACE_LEVEL_FATAL,
" Cannot get index for BAR %I64d\n", accessRange->RangeStart.QuadPart);
EXIT_FN();
return FALSE;
}
adaptExt->pci_bars[iBar].BasePA = accessRange->RangeStart;
adaptExt->pci_bars[iBar].uLength = accessRange->RangeLength;
adaptExt->pci_bars[iBar].bPortSpace = !accessRange->RangeInMemory;
}
}

/* initialize the virtual device */
if (!InitVirtIODevice(DeviceExtension)) {
EXIT_FN();
return FALSE;
}

EXIT_FN();
return TRUE;
}
Expand Down Expand Up @@ -491,53 +504,78 @@ ENTER_FN();
RtlZeroMemory((PVOID)EventNode, sizeof(VirtIOSCSIEventNode));
EventNode->sg.physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &EventNode->event, &fragLen);
EventNode->sg.length = sizeof(VirtIOSCSIEvent);
EXIT_FN();
return SynchronizedKickEventRoutine(DeviceExtension, (PVOID)EventNode);
}

VOID
//
// FORCEINLINE <<---- DO NOT FORCE INLINE
// Wrapper for VioScsiVQLock and VioScsiVQUnlock, so they can be forced inline for optimum performance.
//
VioScsiLockManager(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN OUT PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr,
IN BOOLEAN LockOp
)
{
ENTER_FN();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need ENTER_/EXIT_ for this particular function? It really is just a wrapper for the lock/unlock.

I wonder if it would be more efficient to just put the ENTER/EXIT for isr==TRUE only? like put it on the }. else { conditional at the bottom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but I just wanted to check we left the function clean. Remember, it's for the tracing.

if (!isr) {
// ^^^^^^ Checking NOT isr moved here...
switch (LockOp) {
case VIOSCSI_VQLOCKOP_LOCK:
VioScsiVQLock(DeviceExtension, MessageID, LockHandle, "VioScsiVQLock");
break;
case VIOSCSI_VQLOCKOP_UNLOCK:
VioScsiVQUnlock(DeviceExtension, LockHandle, "VioScsiVQUnlock");
break;
default:
break;
}
}
EXIT_FN();
}

VOID
//FORCEINLINE
FORCEINLINE
VioScsiVQLock(
IN PVOID DeviceExtension,
IN ULONG MessageID,
IN OUT PSTOR_LOCK_HANDLE LockHandle,
IN BOOLEAN isr
IN PVOID InlineFuncName
)
{
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);

ENTER_INL_FN();

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();
EXIT_INL_FN();
}

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

VOID FirmwareRequest(
Expand All @@ -560,6 +598,7 @@ ENTER_FN();
SRB_SET_SRB_STATUS(Srb, SRB_STATUS_BAD_SRB_BLOCK_LENGTH);
RhelDbgPrint(TRACE_LEVEL_ERROR,
" FirmwareRequest Bad Block Length %ul\n", dataLen);
EXIT_FN();
return;
}

Expand Down
Loading