<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=US-ASCII">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: Some more additions questions about the patch:</TITLE>
</HEAD>
<BODY>
<BR>
<BR>
<P><FONT SIZE=2>>-----Original Message-----</FONT>
<BR><FONT SIZE=2>>From: Fab Tillier [<A HREF="mailto:ftillier@silverstorm.com">mailto:ftillier@silverstorm.com</A>]</FONT>
<BR><FONT SIZE=2>>Sent: Monday, September 19, 2005 7:51 AM</FONT>
<BR><FONT SIZE=2>>To: 'Tzachi Dar'; openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>>Subject: RE: Some more additions questions about the patch:</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>> From: Tzachi Dar [<A HREF="mailto:tzachid@mellanox.co.il">mailto:tzachid@mellanox.co.il</A>]</FONT>
<BR><FONT SIZE=2>>> Sent: Sunday, September 18, 2005 1:05 PM</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> While working on checking my patch with a kernel mode client I have found</FONT>
<BR><FONT SIZE=2>>> out that there are some more things that should be changed in the IPOIB</FONT>
<BR><FONT SIZE=2>>> code.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> 1) gp_mem_tracker: It seems that there is a call to</FONT>
<BR><FONT SIZE=2>>__cl_mem_track_start</FONT>
<BR><FONT SIZE=2>>> every time there is an adapter that is being created, and a similar call</FONT>
<BR><FONT SIZE=2>>> when an adapter is being destroyed. It is not clear to me, why this is so</FONT>
<BR><FONT SIZE=2>>> and you are not doing this in the Driver-Entry Driver Unload functions.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>When I made complib a static library, I had forgotten that NDIS drivers</FONT>
<BR><FONT SIZE=2>>could</FONT>
<BR><FONT SIZE=2>>register unload handlers. I added code to let repeated calls to CL_INIT</FONT>
<BR><FONT SIZE=2>>and</FONT>
<BR><FONT SIZE=2>>CL_DEINIT work as long as they were never done in parallel (which they</FONT>
<BR><FONT SIZE=2>>aren't</FONT>
<BR><FONT SIZE=2>>due to how PnP IRPs are handled).</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>> In any case if you keep things as they are, please note that on</FONT>
<BR><FONT SIZE=2>>> __cl_mem_track_stop you have to set gp_mem_tracker to null if the</FONT>
<BR><FONT SIZE=2>>reference</FONT>
<BR><FONT SIZE=2>>> has reached zero, otherwise if the reference has reached zero, things</FONT>
<BR><FONT SIZE=2>>won't</FONT>
<BR><FONT SIZE=2>>> work again.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>This most certainly is a bug, thanks for reporting it.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>> (by the way, if you keep things as they are things won't work in</FONT>
<BR><FONT SIZE=2>>> any case, since there is a race between the reference count and the</FONT>
<BR><FONT SIZE=2>>global</FONT>
<BR><FONT SIZE=2>>> variable :-(, it probably must be done in DriverEntry)</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>The race between reference and destruction isn't an issue due to how the</FONT>
<BR><FONT SIZE=2>>devices</FONT>
<BR><FONT SIZE=2>>are initialized in Windows. I don't think two instances of the same device</FONT>
<BR><FONT SIZE=2>>can</FONT>
<BR><FONT SIZE=2>>receive PnP IRPs simultaneously. It still sucks, though</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>I'm currently adding in functionality needed by the AT stuff that should be</FONT>
<BR><FONT SIZE=2>>there anyway - the first was to put in the list of adapters, which is</FONT>
<BR><FONT SIZE=2>>already</FONT>
<BR><FONT SIZE=2>>committed. The second is to add the unload handler (which I will commit</FONT>
<BR><FONT SIZE=2>>tomorrow), and from that unload handler call CL_DEINIT, and move the</FONT>
<BR><FONT SIZE=2>>CL_INIT</FONT>
<BR><FONT SIZE=2>>call to driver entry (solving the race). I think IPoIB is the only driver</FONT>
<BR><FONT SIZE=2>>that</FONT>
<BR><FONT SIZE=2>>was doing this, and with this change I will remove the changes I had</FONT>
<BR><FONT SIZE=2>>introduced</FONT>
<BR><FONT SIZE=2>>to allow nested calls to CL_INIT and CL_DEINIT, as they are buggy.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>For the kernel, I'm considering going a step further an eliminating the</FONT>
<BR><FONT SIZE=2>>memory</FONT>
<BR><FONT SIZE=2>>tracker - driver verifier has memory leak detection built in, and we might</FONT>
<BR><FONT SIZE=2>>as</FONT>
<BR><FONT SIZE=2>>well just use that instead. The memory leak detection provided by driver</FONT>
<BR><FONT SIZE=2>>verifier doesn't require any sort of recompile of the drivers, which is an</FONT>
<BR><FONT SIZE=2>>extra</FONT>
<BR><FONT SIZE=2>>benefit.</FONT>
<BR><FONT SIZE=2>Does the memory leak detection of the driver verifier also tell where the leak was? If not than there is an advantage to our mechanism, if yes than our mechanism is not really needed.</FONT></P>
<P><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>> 2) I have moved the place in which the adapter is being created. It</FONT>
<BR><FONT SIZE=2>>> should better be created when the number of adapters in the list goes</FONT>
<BR><FONT SIZE=2>>from 0</FONT>
<BR><FONT SIZE=2>>> to 1, This is the case in which there is someone holding the device, the</FONT>
<BR><FONT SIZE=2>>> IPOIB ports are being disabled and re-enabled again.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>You mean when you register the device with NDIS for IOCTL access, or the</FONT>
<BR><FONT SIZE=2>>IPoIB</FONT>
<BR><FONT SIZE=2>>adapter object? Can't we register the device from DriverEntry, and</FONT>
<BR><FONT SIZE=2>>deregister</FONT>
<BR><FONT SIZE=2>>it from the unload handler? The docs for the calls seem to indicate that's</FONT>
<BR><FONT SIZE=2>>the</FONT>
<BR><FONT SIZE=2>>common usage model. I don't think the system will invoke the unload</FONT>
<BR><FONT SIZE=2>>handler</FONT>
<BR><FONT SIZE=2>>until all file handles have been closed. This would simplify things a</FONT>
<BR><FONT SIZE=2>>great</FONT>
<BR><FONT SIZE=2>>deal, wouldn't it?</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>As far as what I saw, DriverUnload will not be called until we close the device, so although I agree that this is simpler, it doesn't work. (you can play with it of course, maybe you will find something).</FONT></P>
<BR>
<P><FONT SIZE=2>>> 3) Please add the case of IRP_MJ_INTERNAL_DEVICE_CONTROL to the</FONT>
<BR><FONT SIZE=2>>function</FONT>
<BR><FONT SIZE=2>>> __ipoib_dispatch. It should be handled just like IRP_MJ_DEVICE_CONTROL.</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>Why don't kernel drivers use IRP_MJ_DEVICE_CONTROL? Why use</FONT>
<BR><FONT SIZE=2>>IRP_MJ_INTERNAL_DEVICE_CONTROL?</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>I'm not sure why IRP_MJ_DEVICE_CONTROL doesn't work for drivers (I believe that it should). I'm still trying to understand if I should use IoGetDeviceObjectPointer to open the device or ZwOpenFile. This might be the cause of the problem (I'm currently using IoGetDeviceObjectPointer).</FONT></P>
<P><FONT SIZE=2>>Thanks,</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>- Fab</FONT>
</P>
</BODY>
</HTML>