[Openib-windows] RE: Some more additions questions about the patch:

[Tzachi Dar]

>-----Original Message-----
>From: Fab Tillier [mailto:ftillier at silverstorm.com]
>Sent: Monday, September 19, 2005 7:51 AM
>To: 'Tzachi Dar'; openib-windows at openib.org
>Subject: RE: Some more additions questions about the patch:
>
>> From: Tzachi Dar [mailto:tzachid at mellanox.co.il]
>> Sent: Sunday, September 18, 2005 1:05 PM
>>
>> While working on checking my patch with a kernel mode client I have found
>> out that there are some more things that should be changed in the IPOIB
>> code.
>>
>> 1)     gp_mem_tracker: It seems that there is a call to
>__cl_mem_track_start
>> every time there is an adapter that is being created, and a similar call
>> when an adapter is being destroyed. It is not clear to me, why this is so
>> and you are not doing this in the Driver-Entry Driver Unload functions.
>
>When I made complib a static library, I had forgotten that NDIS drivers
>could
>register unload handlers.  I added code to let repeated calls to CL_INIT
>and
>CL_DEINIT work as long as they were never done in parallel (which they
>aren't
>due to how PnP IRPs are handled).
>
>> In any case if you keep things as they are, please note that on
>> __cl_mem_track_stop you have to set gp_mem_tracker to null if the
>reference
>> has reached zero, otherwise if the reference has reached zero, things
>won't
>> work again.
>
>This most certainly is a bug, thanks for reporting it.
>
>> (by the way, if you keep things as they are things won't work in
>> any case, since there is a race between the reference count and the
>global
>> variable :-(, it probably must be done in DriverEntry)
>
>The race between reference and destruction isn't an issue due to how the
>devices
>are initialized in Windows.  I don't think two instances of the same device
>can
>receive PnP IRPs simultaneously.  It still sucks, though
>
>I'm currently adding in functionality needed by the AT stuff that should be
>there anyway - the first was to put in the list of adapters, which is
>already
>committed.  The second is to add the unload handler (which I will commit
>tomorrow), and from that unload handler call CL_DEINIT, and move the
>CL_INIT
>call to driver entry (solving the race).  I think IPoIB is the only driver
>that
>was doing this, and with this change I will remove the changes I had
>introduced
>to allow nested calls to CL_INIT and CL_DEINIT, as they are buggy.
>
>For the kernel, I'm considering going a step further an eliminating the
>memory
>tracker - driver verifier has memory leak detection built in, and we might
>as
>well just use that instead.  The memory leak detection provided by driver
>verifier doesn't require any sort of recompile of the drivers, which is an
>extra
>benefit.
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.

>
>> 2)     I have moved the place in which the adapter is being created. It
>> should better be created when the number of adapters in the list goes
>from 0
>> to 1, This is the case in which there is someone holding the device, the
>> IPOIB ports are being disabled and re-enabled again.
>
>You mean when you register the device with NDIS for IOCTL access, or the
>IPoIB
>adapter object?  Can't we register the device from DriverEntry, and
>deregister
>it from the unload handler?  The docs for the calls seem to indicate that's
>the
>common usage model.  I don't think the system will invoke the unload
>handler
>until all file handles have been closed.  This would simplify things a
>great
>deal, wouldn't it?
>
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).


>> 3)     Please add the case of IRP_MJ_INTERNAL_DEVICE_CONTROL to the
>function
>> __ipoib_dispatch. It should be handled just like IRP_MJ_DEVICE_CONTROL.
>
>Why don't kernel drivers use IRP_MJ_DEVICE_CONTROL?  Why use
>IRP_MJ_INTERNAL_DEVICE_CONTROL?
>
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).

>Thanks,
>
>- Fab
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20050919/4aa2ca0d/attachment.html>