<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta name="Generator" content="Microsoft Word 12 (filtered medium)">
<!--[if !mso]><style>v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}
</style><![endif]--><style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
p.emailquote, li.emailquote, div.emailquote
{mso-style-name:emailquote;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:1.0pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle23
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle24
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle25
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle26
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle27
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle28
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle29
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I am wondering now though if there is some need for at least some variant/vestiges of completion-in-isr-instead-of-dpc. Isn’t it the case that in dump mode
– ie either during crash-dump or hibernation - the driver is not allowed to schedule a DPC in it’s ISR to do completions?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">We could put some logic in the ISR to check whether we are in dump mode or not and if we are, do the completions in the ISR itself. After all, if we are in
dump mode, we are single-threaded anyway and won’t have to worry about pre-emption of our StartIo routine by our ISR.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Judy<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Luse, Paul E [mailto:paul.e.luse@intel.com]
<br>
<b>Sent:</b> Thursday, July 18, 2013 7:21 PM<br>
<b>To:</b> Yong Chen; Freyensee, James P; Murray, Kris R; Po-Yen Chang; Judy Brock-SSI; nvmewin@lists.openfabrics.org<br>
<b>Subject:</b> RE: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">All of the issues could of course be fixed, the point I think is that there’s no real reason to fix them….</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Yong Chen [<a href="mailto:Yong.sc.Chen@huawei.com">mailto:Yong.sc.Chen@huawei.com</a>]
<br>
<b>Sent:</b> Thursday, July 18, 2013 6:36 PM<br>
<b>To:</b> Luse, Paul E; Freyensee, James P; Murray, Kris R; Po-Yen Chang; Judy Brock-SSI;
<a href="mailto:nvmewin@lists.openfabrics.org">nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> RE: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I think completion in ISR served some purposes as I used this switch during development.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Unfortunately I found it broken as well during hibernation waking-up:</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas">SntiTranslateStartStopUnit() callback routine issues ProcessIo() directly in ISR , which will bluescreen.</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I was about to put a note in the sources in my code change. So they are multiple ways hitting it as we know it now.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:nvmewin-bounces@lists.openfabrics.org">nvmewin-bounces@lists.openfabrics.org</a> [<a href="mailto:nvmewin-bounces@lists.openfabrics.org">mailto:nvmewin-bounces@lists.openfabrics.org</a>]
<b>On Behalf Of </b>Luse, Paul E<br>
<b>Sent:</b> Thursday, July 18, 2013 4:56 PM<br>
<b>To:</b> Freyensee, James P; Murray, Kris R; Po-Yen Chang; Judy Brock-SSI; <a href="mailto:nvmewin@lists.openfabrics.org">
nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> Re: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">So it was originally in there during early dev to measure system performance impact of completing in the ISR or pushing off to a DPC; general old school rule
of thumb is to minimize ISR work and finish everything else in a DPC to be friendlier to the system. With NVMe completion being so lightweight we figured we could get away without the DPC and it used to work both ways under heavy stress. As both methods
are not always tested (after all it’s a compile switch) clearly its easy to break one of them. I actually ran the tests DPC vs ISR back then and there was no significant impact either way. I was using xperf and I’m sure I shared the results with the other
members of the original team as well – doubt I have them anymore but I’ll look. Either way, at this point in time its probably is a good simplification to pick one method and remove the compile switch for the other. I’d probably stick with the DPC route
as (a) there was no major benefit from finishing in ISR and (b) sounds like its busted now anyway
</span><span style="font-size:11.0pt;font-family:Wingdings;color:#1F497D">J</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:nvmewin-bounces@lists.openfabrics.org">nvmewin-bounces@lists.openfabrics.org</a> [<a href="mailto:nvmewin-bounces@lists.openfabrics.org">mailto:nvmewin-bounces@lists.openfabrics.org</a>]
<b>On Behalf Of </b>Freyensee, James P<br>
<b>Sent:</b> Thursday, July 18, 2013 4:43 PM<br>
<b>To:</b> Murray, Kris R; Po-Yen Chang; Judy Brock-SSI; <a href="mailto:nvmewin@lists.openfabrics.org">
nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> Re: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Out of curiosity, what was the original reason to have the ISR path in the first place? If it is currently in the driver code, there must had been some purpose
to be able to either compile it using an ISR or a DPC.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:nvmewin-bounces@lists.openfabrics.org">nvmewin-bounces@lists.openfabrics.org</a> [<a href="mailto:nvmewin-bounces@lists.openfabrics.org">mailto:nvmewin-bounces@lists.openfabrics.org</a>]
<b>On Behalf Of </b>Murray, Kris R<br>
<b>Sent:</b> Thursday, July 18, 2013 4:29 PM<br>
<b>To:</b> Po-Yen Chang; Judy Brock-SSI; <a href="mailto:nvmewin@lists.openfabrics.org">
nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> Re: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Judy,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I have no problems removing it.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">~Kris</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:nvmewin-bounces@lists.openfabrics.org">nvmewin-bounces@lists.openfabrics.org</a> [<a href="mailto:nvmewin-bounces@lists.openfabrics.org">mailto:nvmewin-bounces@lists.openfabrics.org</a>]
<b>On Behalf Of </b>Po-Yen Chang<br>
<b>Sent:</b> Thursday, July 18, 2013 4:27 PM<br>
<b>To:</b> Judy Brock-SSI; <a href="mailto:nvmewin@lists.openfabrics.org">nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> Re: [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif";color:blue">Judy,</span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif";color:blue">I feel the same way as well. Let's wait for the response from LSI and Intel on this. If they all agree, I will go ahead remove it.</span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif";color:blue">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif";color:blue">Alex</span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div class="MsoNormal" align="center" style="text-align:center">
<hr size="3" width="100%" align="center">
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:nvmewin-bounces@lists.openfabrics.org">nvmewin-bounces@lists.openfabrics.org</a> [<a href="mailto:nvmewin-bounces@lists.openfabrics.org">mailto:nvmewin-bounces@lists.openfabrics.org</a>]
<b>On Behalf Of </b>Judy Brock-SSI<br>
<b>Sent:</b> Thursday, July 18, 2013 4:17 PM<br>
<b>To:</b> Judy Brock-SSI; <a href="mailto:nvmewin@lists.openfabrics.org">nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> [nvmewin] COMPLETE_IN_DPC flag & relationship to NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">So it looks like the reason this problem was not seen before is because it only surfaces when the COMPLETE_IN_DPC compile flag is
<b>not</b> set. In other words, the COMPLETE_IN_ISR path is broken because it accesses our HwDeviceExtension without being synchronized with other paths in the driver which do the same.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">We can either fix the path which does completions in the ISR or get rid of that option entirely. Since it’s generally considered bad practice to do that kind
of work in an ISR because it’s supposed to be as lean and mean as possible, would the team be adverse to getting rid of the logic which optionally allows completions to be handled by the ISR?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">If we insist on retaining it, a) we should come up with a good reason why and b) we should fix it asap because it is definitely not safe to use in its present
form.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Personally I vote for removal - we wouldn’t need the COMPLETE_IN_DPC flag either anymore if we go that route.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Judy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Judy Brock-SSI
<br>
<b>Sent:</b> Thursday, July 18, 2013 6:22 AM<br>
<b>To:</b> Judy Brock-SSI; 'nvmewin@lists.openfabrics.org'<br>
<b>Subject:</b> RE: NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I just thought of another way to handle this problem.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Could we not call
<b>StorPortSynchronizeAccess() </b>with a pointer back to our ProcessIo() routine?<b> </b>ProcessIo would get called before the call to StorPortSynchronizeAccess() returns and this would have the effect of guaranteeing synchronization with our ISR.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">This seems like a much cleaner solution that a lock-acquiring approach.
</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I still don’t know if there are any issues with ProcessIo being called multiple times, from non-StartIo code paths, etc. – would still need to be looked at.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Judy</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Judy Brock-SSI
<br>
<b>Sent:</b> Wednesday, July 17, 2013 10:08 PM<br>
<b>To:</b> <a href="mailto:nvmewin@lists.openfabrics.org">nvmewin@lists.openfabrics.org</a><br>
<b>Subject:</b> NvmeStartio path critical section handling not protected from NVMe ISR?</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">All,
</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Under Windows Server 2012, I’ve seen a crash where NVMeStartIo() gets interrupted by our ISR at a time when it’s in the middle of manipulating a linked list
critical data structure which the ISR then goes on to attempt to manipulate also - which results in a crash. Below is the call stack – see where I’ve inserted the comment</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> “</span><b><span style="font-size:9.0pt;font-family:Wingdings;color:red">ß</span></b><b><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:red">-STARTIO
PATH GETS CLOBBERED BY OUR INTERRUPT HANDLER BECAUSE WE AREN’T HOLDING THE INTERRUPT SPIN LOCK”</span></b><o:p></o:p></p>
<p class="MsoNormal"><b><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:red"> </span></b><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">2: kd> kc</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Call Site</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!RtlpBreakWithStatusInstruction</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiBugCheckDebugBreak</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KeBugCheck2</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KeBugCheckEx</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiBugCheckDispatch</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiFastFailDispatch</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiRaiseSecurityCheckFailure</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!RtlFailFast</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!FatalListEntryError</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!RtlpCheckListEntry</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!InsertTailList</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!NVMeCompleteCmd</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!NVMeIsrMsix</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiInterruptDispatch
</span><b><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:red"> </span></b><b><span style="font-size:9.0pt;font-family:Wingdings;color:red">ß</span></b><b><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:red">-STARTIO
PATH GETS CLOBBERED BY OUR INTERRUPT HANDLER BECAUSE WE AREN’T HOLDING THE INTERRUPT SPIN LOCK</span></b><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!RemoveHeadList</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!NVMeGetCmdEntry</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!ProcessIo</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nvme!NVMeStartIo</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">storport!RaidpAdapterContinueScatterGather</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">hal!HalpAllocateAdapterCallbackV2</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">hal!IoFreeAdapterChannelV2</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">hal!HalAllocateAdapterChannelV2</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">hal!HalBuildScatterGatherListV2</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">storport!RaUnitStartIo</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">storport!RaidUnitCompleteRequest</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">storport!RaidpAdapterRedirectDpcRoutine</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiExecuteAllDpcs</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:#1F497D">nt!KiRetireDpcList</span><o:p></o:p></p>
<p class="MsoNormal"><b><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:red"> </span></b><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">I looked through the code and noticed we never call StorPortAcquireSpinLock to acquire the InterruptLock to protect us from such pre-emption. Another way to achieve
this would be to indicate we run at half-duplex rather than full-duplex but that would degrade the general performance of the driver.
</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">I’m not sure why we didn’t run into this way before now – is there some other re-entrance protection algorithm besides the two above that others are aware of?
If not, I believe we need to fix this asap. Suggestions:</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoListParagraph" style="text-indent:-.25in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">A.</span><span style="font-size:7.0pt;color:black">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Simplest approach is to lock down all of NVMeStartIo as per below (not tested yet) but we almost may as well run half-duplex if we do this:</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:black">1 . At the very the top of NVMeStartIo:</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green">/* we should never be holding the interrupt lock upon entry to NVMeStartIo.</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green"> * Acquire the Interrupt Spin Lock to protect against getting hit by our ISR.
</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green"> */</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:blue">if</span> (NULL == pAdapterExtension->hInterruptLock) {</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> (StorPortAcquireSpinLock(pAdapterExtension,</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> InterruptLock,</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> NULL,</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> &pAdapterExtension->hInterruptLock);</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> }</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:blue">else</span> {</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> ASSERT(FALSE);</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> }</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">2. </span><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:black">At the very the top of
</span><span style="font-size:9.0pt;font-family:"Courier New"">IO_StorPortNotification</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> PNVME_DEVICE_EXTENSION pAE = (PNVME_DEVICE_EXTENSION) pHwDeviceExtension;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:green">/* if we got here from NvmeStartIo we need to release the interrupt lock */</span></span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:blue">if</span> (NULL != pAE->hInterruptLock) {</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> STOR_LOCK_HANDLE hInterruptLockCopy = pAE->hInterruptLock;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> pAE->hInterruptLock = NULL;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> StorPortReleaseSpinLock(pAE, &hInterruptLockCopy);</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> }</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">3. At the very bottom of NVMeStartIo:</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:green">/* if we didn't release the Interrupt Lock in one of the calls to
</span></span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green"> * IO_StorPortNotification above we need to release before we exit NVMEStartIo</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green"> */</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New";color:green"> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:blue">if</span> (NULL != pAE->hInterruptLock) {</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> STOR_LOCK_HANDLE hInterruptLockCopy = pAE->hInterruptLock;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> pAE->hInterruptLock = NULL;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> StorPortReleaseSpinLock(pAE, &hInterruptLockCopy);</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> }</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">
<span style="color:blue">return</span> TRUE;</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:9.0pt;font-family:"Courier New"">}
<span style="color:green">/* NVMeStartIo */</span></span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:10.0pt;font-family:"Courier New";color:green"> </span><o:p></o:p></p>
<p class="MsoListParagraph" style="text-indent:-.25in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">B.</span><span style="font-size:7.0pt;color:black">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Better approach is to just lock ProcessIo(). But code exists in that routine which acquires the StartIo lock – we can’t take locks out of order or we’ll cause deadlock. Right
now that code never gets invoked – what was it for? Do we still need it? Can ProcessIo() get called from non-StartIo Paths? Can it get called multiple times? Not having been involved in the initial development of this driver, I would need to study the flow
to make sure to respect the StorPort lock acquiring/releasing hierarchy rules at all times. If those conversant in the overall developmental history and architecture of this driver could share their thoughts, that would be great.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:black">Judy</span><o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
</div>
</div>
</body>
</html>