Re: [Qemu-devel] MSI interrupt support with vioscsi.c miniport driver

[Vadim Rozenfeld <vrozenfe@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6325 bytes --]

On Thu, 2014-10-30 at 18:28 +0800, Wangting (Kathy) wrote:
> 
> On 2014-10-30 16:48, Vadim Rozenfeld wrote:
> > On Thu, 2014-10-30 at 14:54 +0800, Wangting (Kathy) wrote:
> >>> On Tue, 2014-02-18 at 13:11 -0800, Nicholas A. Bellinger wrote:
> >>>> On Tue, 2014-02-18 at 13:00 -0800, Nicholas A. Bellinger wrote:
> >>>>> On Mon, 2014-02-10 at 11:05 -0800, Nicholas A. Bellinger wrote:
> >>>>>
> >>>>> <SNIP>
> >>>>>
> >>>>>>>>> Hi Yan,
> >>>>>>>>>
> >>>>>>>>> So recently I've been doing some KVM guest performance comparisons
> >>>>>>>>> between the scsi-mq prototype using virtio-scsi + vhost-scsi, and
> >>>>>>>>> Windows Server 2012 with vioscsi.sys (virtio-win-0.1-74.iso) +
> >>>>>>>>> vhost-scsi using PCIe flash backend devices.
> >>>>>>>>>
> >>>>>>>>> I've noticed that small block random performance for the MSFT guest 
> >>>>>>>>> is
> >>>>>>>>> at around ~80K IOPs with multiple vioscsi LUNs + adapters, which 
> >>>>>>>>> ends up
> >>>>>>>>> being well below what the Linux guest with scsi-mq + virtio-scsi is
> >>>>>>>>> capable of (~500K).
> >>>>>>>>>
> >>>>>>>>> After searching through the various vioscsi registry settings, it
> >>>>>>>>> appears that MSIEnabled is being explicitly disabled (0x00000000), 
> >>>>>>>>> that
> >>>>>>>>> is different from what vioscsi.inx is currently defining:
> >>>>>>>>>
> >>>>>>>>> [pnpsafe_pci_addreg_msix]
> >>>>>>>>> HKR, "Interrupt Management",, 0x00000010
> >>>>>>>>> HKR, "Interrupt Management\MessageSignaledInterruptProperties",, 
> >>>>>>>>> 0x00000010
> >>>>>>>>> HKR, "Interrupt Management\MessageSignaledInterruptProperties", 
> >>>>>>>>> MSISupported, 0x00010001, 0
> >>>>>>>>> HKR, "Interrupt Management\MessageSignaledInterruptProperties", 
> >>>>>>>>> MessageNumberLimit, 0x00010001, 4
> >>>>>>>>>
> >>>>>>>>> Looking deeper at vioscsi.c code, I've noticed that MSI_SUPPORTED=0 
> >>>>>>>>> is
> >>>>>>>>> explicitly disabled at build time in SOURCES + vioscsi.vcxproj, as 
> >>>>>>>>> well
> >>>>>>>>> as VioScsiFindAdapter() code always ends setting msix_enabled = 
> >>>>>>>>> FALSE
> >>>>>>>>> here, regardless of MSI_SUPPORTED:
> >>>>>>>>>
> >>>>>>>>>  
> >>>>>>>>> https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/vioscsi/vioscsi.c#L340
> >>>>>>>>>
> >>>>>>>>> Also looking at virtio_stor.c for the raw block driver, 
> >>>>>>>>> MSI_SUPPORTED=1
> >>>>>>>>> appears to be the default setting for the driver included in the 
> >>>>>>>>> offical
> >>>>>>>>> virtio-win iso builds, right..?
> >>>>>>>>>
> >>>>>>>>> Sooo, I'd like to try enabling MSI_SUPPORTED=1 in a test vioscsi.sys
> >>>>>>>>> build of my own, but before going down the WDK development rabbit 
> >>>>>>>>> whole,
> >>>>>>>>> I'd like to better understand why you've explicitly disabled this 
> >>>>>>>>> logic
> >>>>>>>>> within vioscsi.c code to start..?
> >>>>>>>>>
> >>>>>>>>> Is there anything that needs to be addressed / carried over from
> >>>>>>>>> virtio_stor.c in order to get MSI_SUPPORTED=1 to work with vioscsi.c
> >>>>>>>>> miniport code..?
> >>>>>>>
> >>>>>>> Hi Nicholas,
> >>>>>>>
> >>>>>>> I was thinking about enabling MSI in RHEL 6.6 (build 74) but for some
> >>>>>>> reasons decided to keep it disabled until adding mq support.
> >>>>>>>
> >>>>>>>
> >>>>>>> You definitely should be able to turn on MSI_SUPPORTED, rebuild the
> >>>>>>> driver, and switch MSISupported to 1 to make vioscsi driver working in
> >>>>>>> MSI mode.
> >>>>>>>    
> >>>>>>
> >>>>>> Thanks for the quick response.  We'll give MSI_SUPPORTED=1 a shot over
> >>>>>> the next days with a test build on Server 2012 / Server 2008 R2 and see
> >>>>>> how things go..
> >>>>>>
> >>>>>
> >>>>> Just a quick update on progress.
> >>>>>
> >>>>> I've been able to successfully build + load a unsigned vioscsi.sys
> >>>>> driver on Server 2012 with WDK 8.0.
> >>>>>
> >>>>> Running with MSI_SUPPORTED=1 against vhost-scsi results in a significant
> >>>>> performance and efficiency gain, on the order of 100K to 225K IOPs for
> >>>>> 4K block random I/O workload, depending on read/write mix.
> >>>>>
> >>>>
> >>>> One other performance related question..
> >>>>
> >>>> In vioscsi.c:VioScsiFindAdapter() code, the default setting for
> >>>> adaptExt->queue_depth ends up getting set to 32 (pageNum / 4) when
> >>>> indirect mode is enabled in the following bits:
> >>>>
> >>>>     if(adaptExt->indirect) {
> >>>>         adaptExt->queue_depth = max(2, (pageNum / 4));
> >>>>     } else {
> >>>>         adaptExt->queue_depth = pageNum / ConfigInfo->NumberOfPhysicalBreaks 
> >>>> - 1;
> >>>>     }
> >>>>
> >>>> Looking at viostor/virtio_stor.c:VirtIoFindAdapter() code, the default
> >>>> setting for ->queue_depth appears to be 128 (pageNum):
> >>>>
> >>>> #if (INDIRECT_SUPPORTED)
> >>>>     if(!adaptExt->dump_mode) {
> >>>>         adaptExt->indirect = CHECKBIT(adaptExt->features, 
> >>>> VIRTIO_RING_F_INDIRECT_DESC);
> >>>>     }
> >>>>     if(adaptExt->indirect) {
> >>>>         adaptExt->queue_depth = pageNum;
> >>>>     }
> >>>> #else
> >>>>     adaptExt->indirect = 0;
> >>>> #endif
> >>>>
> >>>> Is there a reason for the lower queue_depth for vioscsi vs. viostor..?
> >>>
> >>> It's a horrible work around for the following bug:
> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1013443
> >>>
> >>> I'm going to remove it as soon as found better solution for it.
> >>>
> >>> Best regards,
> >>> Vadim.
> >>>
> >>>
> >> Hi Vadim,
> >>
> >> I have found that Bug 1013443 has been closed with a
> >> resolution of ERRATA.
> >>
> >> The windows device queue must be between 20 and 254
> >> for StorPortSetDeviceQueueDepth to succeed.
> >>
> >> So I have the question that why queue_depth can not be
> >> set to pageNum(128)?
> > 
> > It will create a problem on multi disk setup, when several 
> > disks are attached to the same virtio-scsi pci controller.
> > Adding some sort of manually managed SRBs queue for storing and
> > resubmitting pending requests can solve this problem.
> > 
> > Cheers,
> > Vadim.
> > 
> 
> Is there a patch for it?

Yes, not committed and not fully tested yet. Please, find it attached.

Best,
Vadim.

> 
> >>
> >> Best wishes,
> >> Ting Wang
> >>
> >>>>
> >>>> How about using min(adaptExt->scsi_config.cmd_per_lun, pageNum) instead..?
> >>>>
> >>>> Thanks!
> >>>>
> >>>> -nab
> >>>>
> >>>>
> >>
> >>
> > 
> > 
> > 
> > .
> > 
> 
> 


[-- Attachment #2: 0001-queue-depth254.patch --]
[-- Type: text/x-patch, Size: 7902 bytes --]

>From 3fbee0ab3165657626828f661244ad3145d8b7dc Mon Sep 17 00:00:00 2001
From: Vadim Rozenfeld <vrozenfe@redhat.com>
Date: Sun, 14 Sep 2014 18:57:48 +1000
Subject: [PATCH 1/1] queue depth 254

(cherry picked from commit 540b638bf86bad908b4b7527fdfc634ddca3f0e8)
---
 vioscsi/helper.c  | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
 vioscsi/helper.h  |  7 ++++++-
 vioscsi/vioscsi.c | 40 ++++++++++++++++------------------------
 vioscsi/vioscsi.h |  6 +++++-
 4 files changed, 71 insertions(+), 34 deletions(-)
 mode change 100644 => 100755 vioscsi/helper.c
 mode change 100644 => 100755 vioscsi/helper.h
 mode change 100644 => 100755 vioscsi/vioscsi.c
 mode change 100644 => 100755 vioscsi/vioscsi.h

diff --git a/vioscsi/helper.c b/vioscsi/helper.c
old mode 100644
new mode 100755
index dcbb8d9..6cd6217
--- a/vioscsi/helper.c
+++ b/vioscsi/helper.c
@@ -37,28 +37,60 @@ SynchronizedSRBRoutine(

 ENTER_FN();
     SET_VA_PA();
-    if (virtqueue_add_buf(adaptExt->vq[2],
+    if (IsListEmpty(&adaptExt->list_head) && virtqueue_add_buf(adaptExt->vq[2],
                      &srbExt->sg[0],
                      srbExt->out, srbExt->in,
                      &srbExt->cmd, va, pa) >= 0){
-        virtqueue_kick(adaptExt->vq[2]);
+//        virtqueue_kick(adaptExt->vq[2]);
         return TRUE;
     }
-    Srb->SrbStatus = SRB_STATUS_BUSY;
-    StorPortBusy(DeviceExtension, 2);
-    virtqueue_kick(adaptExt->vq[2]);
-EXIT_ERR();
+    InsertTailList(&adaptExt->list_head, &srbExt->list_entry);
     return FALSE;
 }

-BOOLEAN
+VOID
+ResendSRB(
+    IN PVOID DeviceExtension
+    )
+{
+    PADAPTER_EXTENSION  adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
+    PVOID               va;
+    ULONGLONG           pa;
+    BOOLEAN             kick = FALSE;
+    while (!IsListEmpty(&adaptExt->list_head)) {
+        PSCSI_REQUEST_BLOCK Srb;
+        PSRB_EXTENSION      srbExt;
+        srbExt = (PSRB_EXTENSION)RemoveHeadList(&adaptExt->list_head);
+        Srb = (PSCSI_REQUEST_BLOCK)srbExt->Srb;
+
+        SET_VA_PA();
+
+        if (virtqueue_add_buf(adaptExt->vq[2],
+            &srbExt->sg[0],
+            srbExt->out, srbExt->in,
+            &srbExt->cmd, va, pa) >= 0) {
+            kick = TRUE;
+        } else {
+            InsertHeadList(&adaptExt->list_head, &srbExt->list_entry);
+            return;
+        }
+    }
+    if (kick) {
+        virtqueue_kick(adaptExt->vq[2]);
+    }
+}
+
+VOID
 SendSRB(
     IN PVOID DeviceExtension,
     IN PSCSI_REQUEST_BLOCK Srb
     )
 {
+    PADAPTER_EXTENSION  adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
 ENTER_FN();
-    return StorPortSynchronizeAccess(DeviceExtension, SynchronizedSRBRoutine, (PVOID)Srb);
+    if (StorPortSynchronizeAccess(DeviceExtension, SynchronizedSRBRoutine, (PVOID)Srb)) {
+	virtqueue_kick(adaptExt->vq[2]);
+    }
 EXIT_FN();
 }

@@ -152,6 +184,10 @@ ShutDown(
 {
     PADAPTER_EXTENSION adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
 ENTER_FN();
+    while (!IsListEmpty(&adaptExt->list_head)) {
+        StorPortStallExecution(999);
+    }
+
     VirtIODeviceReset(&adaptExt->vdev);
     StorPortWritePortUshort(DeviceExtension, (PUSHORT)(adaptExt->device_base + VIRTIO_PCI_GUEST_FEATURES), 0);
     if (adaptExt->vq[0]) {
diff --git a/vioscsi/helper.h b/vioscsi/helper.h
old mode 100644
new mode 100755
index b107b42..17e62ef
--- a/vioscsi/helper.h
+++ b/vioscsi/helper.h
@@ -24,12 +24,17 @@
 #include "virtio_pci.h"
 #include "vioscsi.h"

-BOOLEAN
+VOID
 SendSRB(
     IN PVOID DeviceExtension,
     IN PSCSI_REQUEST_BLOCK Srb
     );

+VOID
+ResendSRB(
+    IN PVOID DeviceExtension
+    );
+
 BOOLEAN
 SendTMF(
     IN PVOID DeviceExtension,
diff --git a/vioscsi/vioscsi.c b/vioscsi/vioscsi.c
old mode 100644
new mode 100755
index e2b2e0d..7e78c72
--- a/vioscsi/vioscsi.c
+++ b/vioscsi/vioscsi.c
@@ -351,14 +351,7 @@ ENTER_FN();
     #else
         adaptExt->indirect = 0;
     #endif
-
-        // The windows device queue must be between 20 and 254 for
-        // StorPortSetDeviceQueueDepth to succeed.
-        adaptExt->queue_depth = min(254, max(20, (pageNum / 4)));
-        RhelDbgPrint(TRACE_LEVEL_ERROR, ("breaks_number = %x  queue_depth = %x\n",
-                    ConfigInfo->NumberOfPhysicalBreaks,
-                    adaptExt->queue_depth));
-
+        adaptExt->queue_depth = 254;
         adaptExt->uncachedExtensionVa = StorPortGetUncachedExtension(DeviceExtension, ConfigInfo, allocationSize);
         if (!adaptExt->uncachedExtensionVa) {
             LogError(DeviceExtension,
@@ -373,7 +366,7 @@ ENTER_FN();
         ASSERT(adaptExt->vq[1] != NULL);
         ASSERT(adaptExt->vq[2] != NULL);
     }
-
+    InitializeListHead(&adaptExt->list_head);
     return SP_RETURN_FOUND;
 }

@@ -520,15 +513,14 @@ VioScsiStartIo(
     )
 {
 ENTER_FN();
-    if (PreProcessRequest(DeviceExtension, Srb) ||
-        !SendSRB(DeviceExtension, Srb))
+    if (PreProcessRequest(DeviceExtension, Srb))
     {
-        if(Srb->SrbStatus == SRB_STATUS_PENDING)
-        {
-           Srb->SrbStatus = SRB_STATUS_ERROR;
-        }
         CompleteRequest(DeviceExtension, Srb);
     }
+    else
+    {
+        SendSRB(DeviceExtension, Srb);
+    }
 EXIT_FN();
     return TRUE;
 }
@@ -623,9 +615,9 @@ VioScsiInterrupt(
               Srb->DataTransferLength = srbExt->Xfer;
               Srb->SrbStatus = SRB_STATUS_DATA_OVERRUN;
            }
-         --adaptExt->in_fly;
            CompleteRequest(DeviceExtension, Srb);
         }
+        ResendSRB(DeviceExtension);
         if (adaptExt->tmf_infly) {
            while((cmd = (PVirtIOSCSICmd)virtqueue_get_buf(adaptExt->vq[0], &len)) != NULL) {
               VirtIOSCSICtrlTMFResp *resp;
@@ -814,9 +806,9 @@ VioScsiMSInterrupt (
               Srb->DataTransferLength = srbExt->Xfer;
               Srb->SrbStatus = SRB_STATUS_DATA_OVERRUN;
            }
-           --adaptExt->in_fly;
            CompleteRequest(DeviceExtension, Srb);
         }
+        ResendSRB(DeviceExtension);
         return TRUE;
     }
     return FALSE;
@@ -938,6 +930,7 @@ ENTER_FN();
     RhelDbgPrint(TRACE_LEVEL_VERBOSE, ("<-->%s (%d::%d::%d)\n", DbgGetScsiOpStr(Srb), Srb->PathId, Srb->TargetId, Srb->Lun));

     memset(srbExt, 0, sizeof(*srbExt));
+    srbExt->Srb = Srb;

     cmd = &srbExt->cmd;
     cmd->sc = Srb;
@@ -1035,13 +1028,12 @@ ENTER_FN();
     {
         case SCSIOP_READ_CAPACITY:
         case SCSIOP_READ_CAPACITY16:
-           if (!StorPortSetDeviceQueueDepth( DeviceExtension, Srb->PathId,
-                                     Srb->TargetId, Srb->Lun,
-                                     adaptExt->queue_depth)) {
-              RhelDbgPrint(TRACE_LEVEL_ERROR, ("StorPortSetDeviceQueueDepth(%p, %x) failed.\n",
-                          DeviceExtension,
-                          adaptExt->queue_depth));
-           }
+            if (!StorPortSetDeviceQueueDepth(DeviceExtension, Srb->PathId,
+                Srb->TargetId, Srb->Lun,
+                adaptExt->queue_depth))
+            {
+                RhelDbgPrint(TRACE_LEVEL_ERROR, ("StorPortSetDeviceQueueDepth failed\n"));
+            }
            break;
         default:
            break;
diff --git a/vioscsi/vioscsi.h b/vioscsi/vioscsi.h
old mode 100644
new mode 100755
index 73869a7..45ef911
--- a/vioscsi/vioscsi.h
+++ b/vioscsi/vioscsi.h
@@ -208,6 +208,8 @@ typedef struct vring_desc_alias

 #pragma pack(1)
 typedef struct _SRB_EXTENSION {
+    LIST_ENTRY            list_entry;
+    PSCSI_REQUEST_BLOCK   Srb;
     ULONG                 out;
     ULONG                 in;
     ULONG                 Xfer;
@@ -245,9 +247,11 @@ typedef struct _ADAPTER_EXTENSION {

     TMF_COMMAND           tmf_cmd;
     BOOLEAN               tmf_infly;
-    ULONG                 in_fly;

     PVirtIOSCSIEventNode  events;
+
+    LIST_ENTRY            list_head;
+
 }ADAPTER_EXTENSION, * PADAPTER_EXTENSION;

 #if (MSI_SUPPORTED == 1)
-- 
1.9.3