[nvmewin] Bug Fix Patch - Review Request

[Chang, Alex]

Thanks a lot, Paul, for verifying this issue.

Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]
Sent: Wednesday, November 07, 2012 3:21 PM
To: Chang, Alex; Murray, Kris R; nvmewin at lists.openfabrics.org
Subject: RE: Bug Fix Patch - Review Request

Hi Alex-

I re-ran on a 2 NUMA node 32 core machine and now see a big difference, not quite as big as yours (20% vs 50%) but it's clearly that one LOC that is affecting what storport is doing obviously.  I have a new patch I'll be sending out in the next few days that I mentioned before and I'll go ahead and remove this for now until we figure out why it's causing the issue.  Getting rid of that checked OS assert was a suggestion, its not a requirement so there's no point in leaving it in there unless it does no other harm.  I'll let you all know what comes of looking into further, if anything.

Thanks for catching that!

Thx
Paul



From: Luse, Paul E
Sent: Wednesday, November 07, 2012 9:29 AM
To: 'Chang, Alex'; Murray, Kris R; nvmewin at lists.openfabrics.org
Subject: RE: Bug Fix Patch - Review Request

Hi Alex-

I tried this on the following config and didn't see the performance drop.  Can you please send details of your config (including your iometer ICF file) so I can try again?

System:  8 core I7-2600 desktop (I can switch to a 2 NUMA node 32 core server if needed)
Mem:  8GB
OS:  2008-R2 SP1
Iometer:  built from source, 2008-16-18-RC2
Config:  8 workers, 32 OIO per worker. 100% read, 100% random, 4K
Resukts:  Pretty steady at 187K with or without the change

I also tried the prev patch, msix0 shared, and one that I'm about ready to submit that deals mostly with error handling (just to make sure) and they all behave the same.  Could be that my use of a desktop is masking what you're seeing but before I mess with the big loud server I figured I'd check with you first J

Paul

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Monday, November 05, 2012 11:52 AM
To: Luse, Paul E; Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Bug Fix Patch - Review Request

Hi Paul,

Last Friday, when I ran IOMeter with 4K random reads with latest patch, the performance numbers decreases by half, e.g. number of IOs. After debugging it, the reason is StorPortGetUncachedExtension calling. I am quite sure why. However, if the assertion only happen on check-built Windows 8, I think we should add the calling for that specific case only. I wonder you guys see the performance drop ever?

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Thursday, October 25, 2012 10:53 AM
To: Chang, Alex; Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Bug Fix Patch - Review Request
I think the answer is that we don't care.  Msft suggested that we avoid causing assertions in the checked OS; that's all this does.  We don't use the mem (and don't have to free it) so the only issue that would be present if it failed would be if someone happened to be running on a checked OS when that happened, they'd get an assert in storport just after find adapter and then they could ignore it and move on w/o any further problems....

From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Chang, Alex
Sent: Thursday, October 25, 2012 10:32 AM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] Bug Fix Patch - Review Request

My concern is what if the allocation fails by any chance?

Alex

________________________________
From: Murray, Kris R [mailto:kris.r.murray at intel.com]<mailto:[mailto:kris.r.murray at intel.com]>
Sent: Thursday, October 25, 2012 10:23 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Bug Fix Patch - Review Request
Alex,
Since we don't use the returned pointer I believe there is no need to validate it. The goal is to cause Storport to allocate a DMA adapter object. See the attached email from James Harris for more info.
~Kris

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Thursday, October 25, 2012 10:11 AM
To: Murray, Kris R; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: Bug Fix Patch - Review Request

Hi Kris,

I have a quick question regarding StorPortGetUncachedExtension:
The routine returns a pointer to the allocated buffer, should we validate the pointer before proceeding?

Thanks,
Alex

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Murray, Kris R
Sent: Tuesday, October 16, 2012 10:07 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] Bug Fix Patch - Review Request
Hi all,

The attached NVMe.zip file changes include the below fixes:

*        nvmeStd.c

o   Added a call to StorPortGetUncachedExtension to fix a checked OS assertion

*        nvmeSnti.c

o   Fixed SntiTranslateRead6 function to use the Read mask for the lba instead of the write mask

o   Fixed SntiTranslateWrite6 function to use the correct macro for getting 24 bits from the CDB using the correct offset

*        nvmeSntiTypes.h

o   Updated READ_6_CDB_LBA_MASK definition to match the one for write

o   Fixed WRITE_6_CDB_LBA_OFFSET from 0 to 1

The attached Results.zip file contains results from the test matrix below:

Operating Systems:

*        Windows 7

*        Windows 8

*        Windows Server 2008

*        Windows Server 2012

Tests:

*        IOMeter

*        SCSI Compliance

*        PCMark

Please review the changes, feeling free to send me comments and questions.

Thanks,
~Kris Murray
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20121107/e25e6090/attachment.html>