Message ID | 1414667730.24879.15.camel@oscar |
---|---|
State | New |
Headers | show |
Thank you, I will test it. Best regards, Ting On 2014-10-30 19:15, Vadim Rozenfeld wrote: > 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 >>>>>> >>>>>> >>>> >>>> >>> >>> >>> >>> . >>> >> >> >
Hi Vadim, I test the vioscsi 0001-queue-depth254.patch for virtio-win-0.1-74 with minor modifications about virtio api, because it have not updated to use the linux api. When a VM with four disks in one controller and 4kb 32depth sequential read for each disk at the same time, the tool of iometer which simulates the IO model become no response. If fit the patch into virtio-win-0.1-94, it works well. Is there some bug with the old virtio api, or is there some reason that the patch can not work normally in virtio-win-0.1-74? Best wishes, Ting Wang
On Tue, 2014-12-09 at 16:20 +0800, Wangting (Kathy) wrote: > Hi Vadim, > > I test the vioscsi 0001-queue-depth254.patch for virtio-win-0.1-74 with minor modifications about > > virtio api, because it have not updated to use the linux api. When a VM with four disks in one controller > > and 4kb 32depth sequential read for each disk at the same time, the tool of iometer which simulates > > the IO model become no response. If fit the patch into virtio-win-0.1-94, it works well. > > Is there some bug with the old virtio api, or is there some reason that the patch can not work normally > > in virtio-win-0.1-74? There were a lot of bug-fixes / changes introduced since build 74 (November 2013). Some of them did affect SRB completion and queue depth logic. So do you see any performance improvement after applying the patch? Cheers, Vadim. > > > > Best wishes, > > Ting Wang > >
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