[nvmewin] review comments for patch from Google
Iuliu Rus
iuliur at google.com
Fri Jul 8 12:23:26 PDT 2016
I did, but some of your e-mail servers rejected it, even after changing the
extension.
On Fri, Jul 8, 2016 at 12:20 PM, Robles, Raymond C <
raymond.c.robles at intel.com> wrote:
> Hi Iuliu,
>
>
>
> Did you ever send out your zipped source code?
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Foster, Carolyn D
> *Sent:* Wednesday, June 29, 2016 4:45 PM
> *To:* Iuliu Rus <iuliur at google.com>; Robles, Raymond C <
> raymond.c.robles at intel.com>
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* RE: [nvmewin] review comments for patch from Google
>
>
>
> Hi Iuliu,
>
> Can you please zip your local source files and send it out to the group?
> You might want to encrypt it and provide us with the password.
>
>
>
> Thanks!
>
> Carolyn
>
>
>
> *From:* nvmewin [mailto:nvmewin-bounces at lists.openfabrics.org
> <nvmewin-bounces at lists.openfabrics.org>] *On Behalf Of *Iuliu Rus
> *Sent:* Wednesday, June 29, 2016 4:20 PM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> I think i don't understand how patching works. Did you sync up your local
> copy before looking at it? I did not remove those lines, they are still
> present in my local copy.
>
>
>
> On Wed, Jun 29, 2016 at 3:44 PM, Robles, Raymond C <
> raymond.c.robles at intel.com> wrote:
>
> Hi Iuliu,
>
>
>
> Thanks for the updated patch. After reviewing again, I have some
> additional questions…
>
>
>
> · In ProcessIo, the dpc lock is obtained later in the code than
> originally implemented. Why move the lock acquire down?
>
> · In ProcessIo, it appears that you do not call
> StorPortGetStartIoPerfParams() to obtain the completing MSI-x message
> number. Getting this MSI-x message number ensures that you know which
> interrupt vector will be used to complete this request… and therefore you
> know on which SQ/CQ pair to post the SQE. Was this left out intentionally?
> See sample code below:
>
>
>
>
>
> if (((DeviceExtension->perfData.Flags &
> STOR_PERF_INTERRUPT_MESSAGE_RANGES) == STOR_PERF_INTERRUPT_MESSAGE_RANGES)
> &&
>
> (SrbExtension->pSrb != NULL)) {
>
> Perf.Size = sizeof(STARTIO_PERFORMANCE_PARAMETERS);
>
> StorPortGetStartIoPerfParams((PVOID)DeviceExtension,
>
> (PSCSI_REQUEST_BLOCK)pSrbExtension->pSrb,
>
> &Perf);
>
> StorStatus = NVMeMapMsiToQueue(DeviceExtension,
>
> Perf.MessageNumber,
>
> &SubQueue,
>
> &CplQueue);
>
>
>
>
>
> · When setting up concurrent channels, I do not see the Storport
> Perf Opts changes from Suman’s patch (Samsung). The Perf Opts changes
> replace the old learning mode and drive the source core interrupt steering
> in the driver. Without it, the concurrent channels changes will not be
> optimized correctly. See reference to Perf Opts setup code (directly from
> OFA source trunk in NVMeInitialize) below:
>
>
>
>
>
> if (pAE->ntldrDump == FALSE) {
>
> /* Enumerate granted MSI/MSI-X messages if there is any */
>
> if (NVMeEnumMsiMessages(pAE) == FALSE) {
>
> NVMeFreeBuffers(pAE);
>
> return (FALSE);
>
> }
>
> perfQueryData.Version = STOR_PERF_VERSION;
>
> perfQueryData.Size = sizeof(PERF_CONFIGURATION_DATA);
>
> Status = StorPortInitializePerfOpts(pAE, TRUE, &perfQueryData);
>
> ASSERT(STOR_STATUS_SUCCESS == Status);
>
> if (STOR_STATUS_SUCCESS == Status) {
>
>
>
> perfData.Version = STOR_PERF_VERSION;
>
> perfData.Size = sizeof(PERF_CONFIGURATION_DATA);
>
>
>
> /* Allow optimization of storport DPCs */
>
> if (perfQueryData.Flags & STOR_PERF_DPC_REDIRECTION) {
>
> perfData.Flags = STOR_PERF_DPC_REDIRECTION;
>
>
>
> /* Allow optimization of Interrupt Redirection and
> Group Affinity */
>
> if ((perfQueryData.Flags &
> (STOR_PERF_INTERRUPT_MESSAGE_RANGES | STOR_PERF_ADV_CONFIG_LOCALITY)) ==
>
> (STOR_PERF_INTERRUPT_MESSAGE_RANGES |
> STOR_PERF_ADV_CONFIG_LOCALITY)){
>
> perfData.Flags |=
> STOR_PERF_INTERRUPT_MESSAGE_RANGES;
>
> perfData.Flags |= STOR_PERF_ADV_CONFIG_LOCALITY;
>
>
>
> perfData.FirstRedirectionMessageNumber = 0;
>
> perfData.LastRedirectionMessageNumber =
> (pRMT->NumMsiMsgGranted - 1);
>
> perfData.MessageTargets = pAE->pArrGrpAff;
>
> }
>
> }
>
>
>
> Status = StorPortInitializePerfOpts(pAE, FALSE, &perfData);
>
> ASSERT(STOR_STATUS_SUCCESS == Status);
>
> if (STOR_STATUS_SUCCESS == Status){
>
> pAE->IsMsiMappingComplete = TRUE;
>
> }
>
> }
>
>
>
> /* Call StorPortPassiveInitialization to enable passive init */
>
> StorPortEnablePassiveInitialization(pAE, NVMePassiveInitialize);
>
>
>
> return (TRUE);
>
> } else {
>
>
>
>
>
>
>
> Thank you…
>
> Ray
>
>
>
>
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com]
> *Sent:* Wednesday, June 29, 2016 10:56 AM
> *To:* Thomas Freeman <thomas.freeman at hgst.com>
> *Cc:* Robles, Raymond C <raymond.c.robles at intel.com>; nvmewin <
> nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Sorry about Thomas. I resolved that conflict but i think i sent you the
> patch pre-resolve by mistake. Here is the correct patch. I also triggered
> the RecoveryDpc from the debugger, to make sure i have coverage on that. I
> don't think the normal tests cover it.
>
>
>
>
>
> On Wed, Jun 29, 2016 at 7:25 AM, Thomas Freeman <thomas.freeman at hgst.com>
> wrote:
>
> Iuliu,
>
> It looks like the patch file has an unresolved conflict:
>
>
>
> +<<<<<<< .mine
>
> + ReleaseDpcSpinLocks(pAE, dpcLocks);
>
> + } /* reset the controller */
>
> + if (pAE->NumDpc > MAX_DPC) {
>
> + StorPortFreePool(pHwDeviceExtension,
> dpcLocks);
>
> + }
>
> +||||||| .r137
>
> + StorPortReleaseSpinLock(pAE, &startLockhandle);
>
> + } /* reset the controller */
>
> +=======
>
> pAE->DeviceRemovedDuringIO = TRUE;
>
> #if (NTDDI_VERSION > NTDDI_WIN7)
>
> if (pAE->Timerhandle != NULL) {
>
> @@ -2634,6 +2680,7 @@
>
> NVMeFreeBuffers(pAE);
>
> StorPortResume(pAE);
>
> } /* reset the controller */
>
> +>>>>>>> .r139
>
>
>
>
>
>
>
> *Tom Freeman*
>
> *Software Engineer, Device Manager and Driver Development*
>
> Western Digital Corporation
>
> e. Thomas.freeman at hgst.com
>
> o. +1-507-322-2311
>
>
>
> [image: cid:image001.jpg at 01D1D1F7.245A73D0]
>
>
>
> *From:* nvmewin [mailto:nvmewin-bounces at lists.openfabrics.org] *On Behalf
> Of *Iuliu Rus
> *Sent:* Monday, June 27, 2016 11:12 AM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>
>
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Done.
>
>
>
> On Mon, Jun 27, 2016 at 8:52 AM, Robles, Raymond C <
> raymond.c.robles at intel.com> wrote:
>
> Hi Suman,
>
>
>
> Thank you for checking on your patches. I was unaware that your latest
> patch for IOCTL Surprise Removal was not applied correctly. I’ve just
> applied that patch again and the trunk should now be at revision 138. Could
> you please double check to ensure you patch is applied properly and let me
> know? Thank you!!!
>
>
>
> Hi Iuliu,
>
>
>
> Sorry about the thrash… but I think we’ve finally got the trunk up to date
> to the latest (once confirmed by Suman from Samsung). Once confirmed,
> please rebase you trunk and add your modifications to that latest revision
> (138). Thank you!!!
>
>
>
> Thanks…
> Ray
>
>
>
> *From:* SUMAN PRAKASH B [mailto:suman.p at samsung.com]
> *Sent:* Monday, June 27, 2016 1:12 AM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>; Iuliu Rus <
> iuliur at google.com>
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* Re: Re: [nvmewin] review comments for patch from Google
>
>
>
> Hi Ray,
>
>
>
> Samsung submitted 3 patches. Revision 134 is first patch, Revision 136 and
> 137 is second patch.
>
> We submitted third patch for Handling device Surprise removal in IOCTL
> path which was approved by HGST and Intel on 1st June. Please find attached
> the mail thread for reference.
>
> The third patch is not yet checked-in to trunk.
>
>
>
> Thanks,
>
> Suman
>
>
>
> ------- *Original Message* -------
>
> *Sender* : Robles, Raymond C<raymond.c.robles at intel.com>
>
> *Date* : Jun 27, 2016 12:19 (GMT+05:30)
>
> *Title* : Re: [nvmewin] review comments for patch from Google
>
>
>
> Hi Iuliu,
>
>
>
> I apologize for the delay in responding. I was performing a manual diff of
> the last several merges to complete the investigation. Here is what I
> found.
>
>
>
> The latest revision on the OFA source trunk is actually 137 (refer to my
> screenshot below). I’ve manually gone back and made sure that all the
> patches submitted are in this latest revision. I found no errors or issues.
> The latest trunk is up to date with all of the patches. If you were trying
> to apply your patch yourself, you cannot. Only myself (OFA co-chair) and
> Carolyn Foster (OFA source maintainer) have write permissions to the repo.
>
>
>
> Each patch listed below is applied to trunk. The only think you need to
> insure is that your latest concurrent channels modifications are based off
> the 137 revision. If you have any questions, please me know.
>
>
>
> Here is the screenshot of all the patches I’ve checked…
>
>
>
>
>
> [image: cid:image002.png at 01D1D1F7.245A73D0]
>
>
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* nvmewin [mailto:nvmewin-bounces at lists.openfabrics.org] *On Behalf
> Of *Robles, Raymond C
> *Sent:* Friday, June 17, 2016 12:08 PM
> *To:* Iuliu Rus <iuliur at google.com>
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Thanks Iuliu. I’m currently investigating the issue. Will let everyone
> know when the trunk is updated.
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com <iuliur at google.com>]
> *Sent:* Friday, June 17, 2016 10:29 AM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>
> *Cc:* Thomas Freeman <thomas.freeman at hgst.com>; nvmewin <
> nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> I don't see any changes when i try to update. My svn says : completed at
> revision 137.
>
>
>
> On Thu, Jun 16, 2016 at 2:42 PM, Robles, Raymond C <
> raymond.c.robles at intel.com> wrote:
>
> Hi Iuliu- Please rebase your patch with the latest OFA trunk source. If
> you do not see any updates during your rebase, please let me know and I’ll
> investigate. Thanks!
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com]
> *Sent:* Thursday, June 16, 2016 2:15 PM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>
> *Cc:* Thomas Freeman <thomas.freeman at hgst.com>; nvmewin <
> nvmewin at lists.openfabrics.org>
>
>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Let me know if i have to sync to the latest and re-do the patch.
>
>
>
> On Thu, Jun 16, 2016 at 2:10 PM, Iuliu Rus <iuliur at google.com> wrote:
>
> We already have a pool of locks - the dpc locks, which is what i'm using
> in my latest patch. You cannot create spinlocks directly in a storport
> driver, the only option is to use either the StartIoLock, the dpc lock, the
> interrupt lock or one of the MSI locks.
>
>
>
> On Thu, Jun 16, 2016 at 2:04 PM, Robles, Raymond C <
> raymond.c.robles at intel.com> wrote:
>
> Hello,
>
>
>
> Here are my thoughts…
>
>
>
> In short, I agree with Thomas’s assessment.
>
>
>
> Concurrent channels allows Storport to call StartIo concurrently… and with
> no locks. Therefore the locking of critical sections is now the
> responsibility of the miniport.
>
>
>
> Any solution proposed should protect critical resources (cmd id pool, cmd
> entries, doorbells) with a lock that is granular enough for our driver, but
> not at the visibility of the OS or Storport. Acquiring the StartIo lock to
> protect resources in our driver only is too broad of a lock. My suggestion
> would be to create a pool of locks (one for every SQ/CQ pair) that is
> maintained within our driver. Again, crashdump mode is a non-issue because
> it is single threaded.
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Thomas Freeman [mailto:thomas.freeman at hgst.com]
> *Sent:* Thursday, June 16, 2016 1:51 PM
> *To:* Iuliu Rus <iuliur at google.com>
> *Cc:* Robles, Raymond C <raymond.c.robles at intel.com>; nvmewin <
> nvmewin at lists.openfabrics.org>
> *Subject:* RE: [nvmewin] review comments for patch from Google
>
>
>
> Hi Iuliu,
>
> Here my view, I hope someone else can chime in to verify or correct what
> I'm saying
>
>
>
> The purpose of the concurrent channels change is to prevent Storport from
> acquiring the StartIo lock before calling Startio. This allows multiple
> simultaneous calls to StartIo. If you require ProcessIo get the StartIo
> lock, you are replicating what Storport used to - it defeats the point of
> concurrent channels.
>
>
>
> Here is my suggestion for what needs to be changed
>
> 1. There is no need for StartIo Locks - remove the requests to acquire them
>
> 2. Acquire the Dpc locks in the following situations
>
> -ProcessIo, before calling GetCmdEntry - if there is queue sharing
>
> -ProcessIo, before calling IssueCmd - always
>
> -IoCompletionRoutine, before calling NVMeCompleteCmd - if there is
> queue sharing
>
>
>
> -recoveryDpcRoutine - get all of the Dpc locks
>
>
>
>
>
> Here is my reasoning
>
> -GetCmdEntry and NVMeCompleteCmd both access the FreeQList. With queue
> sharing, these could run simultaneously and have a collision when they
> update the list.
>
> -IssueCmd. With queue sharing, different cpu's could attempt to put
> entries on the queue simultaneously. But, to make RecoveryDpcRoutine work,
> this must be protected by the dpcLock whether or not there is queue
> sharing.
>
>
>
> I think this will address the deadlock problem I reported also.
>
>
>
> Regards,
>
> *Tom Freeman*
>
> *Software Engineer, Device Manager and Driver Development*
>
> Western Digital Corporation
>
> e. Thomas.freeman at hgst.com
>
> o. +1-507-322-2311
>
>
>
> [image: cid:image001.jpg at 01D1D1F7.245A73D0]
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com <iuliur at google.com>]
> *Sent:* Thursday, June 16, 2016 2:42 PM
> *To:* Thomas Freeman <thomas.freeman at hgst.com>
> *Cc:* Robles, Raymond C <raymond.c.robles at intel.com>; nvmewin <
> nvmewin at lists.openfabrics.org>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Here's my revised patch for concurrent channels. The synchronization rules
> are now as follows:
>
> 1. Init is single threaded so it doesn't matter if acquirelock is true or
> false
>
> 2. Anything called from the DPC has to call ProcessIo with false, since
> the dpc spinlock is already acquired.
>
> 3. Anything called from StartIo has to call ProcessIo with true, since now
> the startio is not automatically acquired.
>
>
>
> Thomas, i think the bug that you found is because the ProcessIo was
> incorrectly called with TRUE instead of FALSE.
>
> Let me know what are your thoughts on this.
>
>
>
> On Thu, Jun 16, 2016 at 10:41 AM, Iuliu Rus <iuliur at google.com> wrote:
>
> Guys, am i OK continuing with the patch on the current code base or do you
> want me to wait until you pull in some more changes?
>
>
>
> On Wed, Jun 15, 2016 at 8:37 AM, Thomas Freeman <thomas.freeman at hgst.com>
> wrote:
>
> Ray,
>
> It appears that the latest patch file from Iuliu is applied to revision
> 137. I don’t see Suman’s changes in that revision.
>
>
>
> As an example, the use of StorPortRequestTimer is not in nvmeStat.c.
>
>
>
> I thought there were some newer revisions than 137, but 137 is the latest
> I’m able to download.
>
> 137 is also the revision that appears when I go to
> https://svn.openfabrics.org/svnrepo/nvmewin/
>
>
>
> SVN is giving me some troubles, so I can’t tell for sure if this is my
> environment or a server issue.
>
>
>
> *Tom Freeman*
>
> *Software Engineer, Device Manager and Driver Development*
>
> Western Digital Corporation
>
> e. Thomas.freeman at hgst.com
>
> o. +1-507-322-2311
>
>
>
> [image: cid:image003.jpg at 01D1D1F7.245A73D0]
>
>
>
> *From:* nvmewin [mailto:nvmewin-bounces at lists.openfabrics.org] *On Behalf
> Of *Robles, Raymond C
> *Sent:* Friday, June 10, 2016 12:23 PM
> *To:* Iuliu Rus <iuliur at google.com>; suman.p at samsung.com
> *Cc:* nvmewin <nvmewin at lists.openfabrics.org>
>
>
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Hi Iuliu,
>
>
>
> If you send out the updated patch with the protection in place for both
> list entries and doorbells, I can run it on an Intel NVMe SSD (P3700) and
> see if there is any performance impact (up or down).
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com <iuliur at google.com>]
> *Sent:* Friday, June 10, 2016 10:17 AM
> *To:* suman.p at samsung.com
> *Cc:* Robles, Raymond C <raymond.c.robles at intel.com>; Judy Brock <
> judy.brock at samsung.com>; nvmewin <nvmewin at lists.openfabrics.org>
> *Subject:* Re: RE: review comments for patch from Google
>
>
>
> we are running this in a virtual machine, and we do see a perf difference
> with concurrent channels. We did not observe any performance drop with
> synchronized access to the list entry.
>
>
>
> On Fri, Jun 10, 2016 at 7:15 AM, SUMAN PRAKASH B <suman.p at samsung.com>
> wrote:
>
> Hi Iuliu,
>
>
>
> As you have the implementation in place, can you let us know if there is
> any performance improvement when STOR_PERF_CONCURRENT_CHANNELS is set in
> case of #core and #queues not matching.
>
> Also, if there is any drop in performance if the list entry and doorbell
> is protected after setting STOR_PERF_CONCURRENT_CHANNELS.
>
>
>
> Perhaps a bit off topic:
>
> Even in case when #cores and #queues are same and when
> STOR_PERF_CONCURRENT_CHANNELS is set, we did not observe any performance
> gain. Maybe we are hitting the max performance from device even without
> setting STOR_PERF_CONCURRENT_CHANNELS. Can you please let us know your
> observation?
>
>
>
> Thanks,
>
> Suman
>
>
>
> ------- *Original Message* -------
>
> *Sender* : Robles, Raymond C<raymond.c.robles at intel.com>
>
> *Date* : Jun 10, 2016 08:30 (GMT+05:30)
>
> *Title* : RE: review comments for patch from Google
>
>
>
> Hi Judy,
>
>
>
> Thanks for the additional info. I agree with the assessment. My statement
> was directed at the general direction of the solution… and the fact that I
> think option 1 is not as robust as option 2.
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Judy Brock [mailto:judy.brock at samsung.com]
> *Sent:* Thursday, June 09, 2016 5:14 PM
> *To:* Robles, Raymond C <raymond.c.robles at intel.com>; Iuliu Rus <
> iuliur at google.com>; suman.p at samsung.com
> *Cc:* nvmewin at lists.openfabrics.org
> *Subject:* RE: review comments for patch from Google
>
>
>
> Hi Ray et al,
>
> [Suman wrote] >> Protect the listentry and doorbell before access/update
>
> >>Do not set STOR_PERF_CONCURRENT_CHANNELS
> when number of cores and queues are not same.
>
>
>
> Ray wrote] >>In the case Suman points out, concurrent channels should
> still be enabled even if the number of cores and queues do not match. This
> is especially true for large core count systems (>128 logical cpu cores)
> but devices may only support 32 queues or less
>
>
>
> I think Suman was saying, we have two options:
>
> 1. either don’t enable it at all if # cores != #queues or
>
> 2. protect both the list entries and doorbell writes before any
> accesses/updates if #cores != #queues.
>
>
>
> So we will need to check to see if we need to use locks or not in the
> performance path based on whether #cores and # queues DO match or not if we
> want to enable concurrent channels no matter what. It’s probably better
> than option 1) above if this is a configuration that can come up frequently.
>
>
>
> Thanks,
>
> Judy
>
>
>
>
>
> *From:* nvmewin [mailto:nvmewin-bounces at lists.openfabrics.org
> <nvmewin-bounces at lists.openfabrics.org>] *On Behalf Of *Robles, Raymond C
> *Sent:* Thursday, June 09, 2016 2:32 PM
> *To:* Iuliu Rus; suman.p at samsung.com
> *Cc:* nvmewin at lists.openfabrics.org
> *Subject:* Re: [nvmewin] review comments for patch from Google
>
>
>
> Hi Iuliu/Suman,
>
>
>
> I’m of the opinion that any solution provided should be as robust as
> possible. In the case Suman points out, concurrent channels should still be
> enabled even if the number of cores and queues do not match. This is
> especially true for large core count systems (>128 logical cpu cores) but
> devices may only support 32 queues or less.
>
>
>
> Thank you…
>
> Ray
>
>
>
> *From:* Iuliu Rus [mailto:iuliur at google.com]
> *Sent:* Thursday, June 09, 2016 2:09 PM
> *To:* suman.p at samsung.com
> *Cc:* Robles, Raymond C <raymond.c.robles at intel.com>;
> nvmewin at lists.openfabrics.org
> *Subject:* Re: review comments for patch from Google
>
>
>
> Which one do you prefer? Our google driver has the synchronization in
> place already for a different reason. Or, I can follow your second
> suggestion.
>
>
>
> On Thu, Jun 9, 2016 at 7:39 AM, SUMAN PRAKASH B <suman.p at samsung.com>
> wrote:
>
> Hi Iuliu,
>
>
>
> Please find my review comments below:
>
>
>
> 1. If the number of Cores and Queues supported by device are not same,
> then setting STOR_PERF_CONCURRENT_CHANNELS will corrupt the submission
> queue. For example, if device has 4 queues and connected to target PC which
> has 8 logical processors, and if STOR_PERF_CONCURRENT_CHANNELS is set, all
> the 8 logical processors will get startIo calls concurrently. As per OFA
> driver NUMA design, the IO Queue 1 will be shared by core 0 and core 4.
> There is possibility that core 0 and core 4 will get startio() calls
> concurrently and will access/update the listenty and queue 1 doorbell
> register at same time, which will lead to listentry and doorbell register
> corruption.
> This problem can be address by following ways:
> a. Protect the listentry and doorbell before access/update.
> b. Do not set STOR_PERF_CONCURRENT_CHANNELS when number of cores and
> queues are not same.
>
>
>
> 2. We can use pRMT->NumActiveCores which is updated in NVMeEnumNumaCores()
> instead of using KeQueryActiveProcessorCountEx/KeQueryActiveProcessorCount.
>
>
>
> Please let us know your opinion.
>
>
>
> Thanks,
> Suman
>
>
>
> ------- *Original Message* -------
>
> *Sender* : Robles, Raymond C<raymond.c.robles at intel.com>
>
> *Date* : Jun 09, 2016 03:56 (GMT+05:30)
>
> *Title* : Re: [nvmewin] Happy New Year... and status update
>
>
>
> Thank you Iuliu!
>
>
>
> Reviewing companies, please try to target the review completion for 2
> weeks from today (6/22/16).
>
>
>
> ...
>
> [Message clipped]
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160708/3984fae0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image003.jpg
Type: image/jpeg
Size: 2937 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160708/3984fae0/attachment.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image002.png
Type: image/png
Size: 236073 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160708/3984fae0/attachment.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.jpg
Type: image/jpeg
Size: 2938 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160708/3984fae0/attachment-0001.jpg>
More information about the nvmewin
mailing list