[Openib-windows] [RFC] changing kernel complib to be static l ibrary

Leonid Keller leonid at mellanox.co.il
Wed Jul 13 10:05:54 PDT 2005


i don't like the idea.
Windows' kernel consists of DLLs and they change it with service packs
anyway ...
And what about IBAL.sys - it is also used now by several components ?

Using a static library can bring us to kind of "DLL mess" problem, when each
driver works with its own version of COMPLIB and a bug, fixed in one
version, continues to be present in others.

May be we can use another solutions, say:
	- add versioning to COMPLIB , so a new HCA driver will fail loading
upon unappropriate COMPLIB version;
	- add checking of build_types of the components (also on startup);
	- (the worst case) to copy all the files to system\drivers upon
driver update and require reboot! :(


-----Original Message-----
From: Fab Tillier [mailto:ftillier at silverstorm.com]
Sent: Tuesday, July 12, 2005 11:32 PM
To: openib-windows at openib.org
Subject: [Openib-windows] [RFC] changing kernel complib to be static
library


Folks,

Currently, the kernel level component library driver is a kernel DLL.  This
places a few restrictions on the user:

1. All kernel drivers must be of the same build type as complib.  Unmatched
drivers can put the system in a state where it crashes on boot, and safe
mode
doesn't always help.  Recovering from this is really difficult, and it's
almost
easier to just re-install the system (yes, that's how bad this is).

2. Driver updates don't always properly track DLL dependencies, so updating
the
complib DLL might not take effect, resulting in odd behavior.  Say someone
updates the HCA driver, which installs a new version of complib driver to
disk.
If there are any existing drivers loaded with a dependency on complib, the
complib driver won't be unloaded.  When the HCA driver starts, it will end
up
using the existing complib driver, not the updated one.  This could be
messy,
depending on the update to complib.

The fix for this is to make complib a static library, but the result is
larger
binary footprint (1% total, but up to 50% for some drivers for release
builds),
and likely a larger memory footprint due to things being duplicated.  This
is
exacerbated in debug builds, where debug code is duplicated in each driver
(e.g.
the memory tracker).

Lastly, changing to a static library requires changing drivers to explicitly
initialize and cleanup global complib stuff (like the memory tracker and
object
manager).

Below is a patch that does this.  Please let me know if this seems
worthwhile,
and if so, if there are any issues or improvements.

Thanks,

- Fab

Index: ulp/ipoib/kernel/SOURCES
===================================================================
--- ulp/ipoib/kernel/SOURCES	(revision 36)
+++ ulp/ipoib/kernel/SOURCES	(working copy)
@@ -11,7 +11,7 @@
 INCLUDES=..;..\..\..\inc;..\..\..\inc\kernel;
 
 C_DEFINES=$(C_DEFINES) -DNDIS_MINIPORT_DRIVER -DNDIS_WDM=1 \
-	-DDEPRECATE_DDK_FUNCTIONS -DNDIS51_MINIPORT
+	-DDEPRECATE_DDK_FUNCTIONS -DNDIS51_MINIPORT -DNEED_CL_OBJ
 
 TARGETLIBS= \
 	$(TARGETPATH)\*\complib.lib \
Index: ulp/ipoib/kernel/ipoib_adapter.c
===================================================================
--- ulp/ipoib/kernel/ipoib_adapter.c	(revision 36)
+++ ulp/ipoib/kernel/ipoib_adapter.c	(working copy)
@@ -129,9 +129,17 @@
 
 	IPOIB_ENTER( IPOIB_DBG_INIT );
 
+	if( !NT_SUCCESS( CL_INIT ) )
+	{
+		IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR,
+			("cl_init failed.\n") );
+		return IB_ERROR;
+	}
+
 	p_adapter = cl_zalloc( sizeof(ipoib_adapter_t) );
 	if( !p_adapter )
 	{
+		CL_DEINIT;
 		IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR, 
 			("Failed to allocate ipoib_adapter_t (%d bytes)",
 			sizeof(ipoib_adapter_t)) );
@@ -455,6 +463,8 @@
 
 	cl_free( p_adapter );
 
+	CL_DEINIT;
+
 	IPOIB_EXIT( IPOIB_DBG_INIT );
 }
 
Index: ulp/ipoib/kernel/ipoib.rc
===================================================================
--- ulp/ipoib/kernel/ipoib.rc	(revision 40)
+++ ulp/ipoib/kernel/ipoib.rc	(working copy)
@@ -26,7 +26,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id$
+ * $Id: ipoib.rc 1.13 05/07/12 10:46:54-07:00
ftillier at orbuild.infiniconsys.com
$
  */
 
 
Index: ulp/ipoib/kernel/ipoib_driver.c
===================================================================
--- ulp/ipoib/kernel/ipoib_driver.c	(revision 36)
+++ ulp/ipoib/kernel/ipoib_driver.c	(working copy)
@@ -516,6 +516,7 @@
 
 	if( medium_index == medium_array_size ) /* Never found it */
 	{
+		CL_DEINIT;
 		IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR, ("No supported media.\n")
);
 		return NDIS_STATUS_UNSUPPORTED_MEDIA;
 	}
Index: ulp/srp/kernel/SOURCES
===================================================================
--- ulp/srp/kernel/SOURCES	(revision 36)
+++ ulp/srp/kernel/SOURCES	(working copy)
@@ -16,9 +16,10 @@
 
 !if defined(DDK_TARGET_OS) && "$(DDK_TARGET_OS)"=="WinXP"
 # storport.h in WinXP DDK already have "..._ALIASES" definition
-C_DEFINES=$(C_DEFINES) -DDEPRECATE_DDK_FUNCTIONS -DWinXP
+C_DEFINES=$(C_DEFINES) -DDEPRECATE_DDK_FUNCTIONS -DWinXP -DNEED_CL_OBJ
 !else
-C_DEFINES=$(C_DEFINES) -DDEPRECATE_DDK_FUNCTIONS -DSTOR_USE_SCSI_ALIASES
+C_DEFINES=$(C_DEFINES) -DDEPRECATE_DDK_FUNCTIONS -DSTOR_USE_SCSI_ALIASES \
+	-DNEED_CL_OBJ
 !endif
 
 TARGETLIBS= \
Index: ulp/srp/kernel/srp_driver.c
===================================================================
--- ulp/srp/kernel/srp_driver.c	(revision 36)
+++ ulp/srp/kernel/srp_driver.c	(working copy)
@@ -204,12 +204,20 @@
 	IN              DRIVER_OBJECT               *p_drv_obj,
 	IN              UNICODE_STRING              *p_registry_path )
 {
-	ULONG                       status =
(ULONG)STATUS_INSUFFICIENT_RESOURCES;
+	ULONG                       status;
 	HW_INITIALIZATION_DATA      hw_data;
 	cl_status_t                 cl_status;
 
 	SRP_ENTER( SRP_DBG_PNP );
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		SRP_TRACE_EXIT( SRP_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	gp_drv_obj = p_drv_obj;
 
 	cl_obj_construct( &g_drv_obj, SRP_OBJ_TYPE_DRV );
@@ -271,10 +279,16 @@
 		}
 		else
 		{
+			CL_DEINIT;
 			SRP_TRACE( SRP_DBG_ERROR,
 				("StorPortInitialize returned 0x%x.\n",
status)
);
 		}
 	}
+	else
+	{
+		CL_DEINIT;
+		status = (ULONG)STATUS_INSUFFICIENT_RESOURCES;
+	}
 
 	SRP_TRACE_EXIT( SRP_DBG_PNP, ("DriverEntry returning status of
0x%x.\n",
status) );
 	return status;
@@ -291,6 +305,8 @@
 	SRP_TRACE( SRP_DBG_VERBOSE, ("Driver Object ref_cnt = %d\n",
g_drv_obj.ref_cnt) );
 	cl_obj_destroy( &g_drv_obj );
 
+	CL_DEINIT;
+
 	/* Invoke the port driver's unload routine. */
 	SRP_TRACE( SRP_DBG_DEBUG, ("Invoking the port driver's unload
routine.\n") );
 	gpfn_unload( p_drv_obj );
Index: tests/alts/kernel/alts_driver.c
===================================================================
--- tests/alts/kernel/alts_driver.c	(revision 36)
+++ tests/alts/kernel/alts_driver.c	(working copy)
@@ -133,6 +133,7 @@
 	IN				PDRIVER_OBJECT
p_driver_obj,
 	IN				PUNICODE_STRING
p_registry_path )
 {
+	NTSTATUS			status;
 #ifdef _DEBUG_
 	static boolean_t	exit = FALSE;
 #endif
@@ -150,6 +151,14 @@
 	}
 #endif
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		ALTS_TRACE_EXIT( ALTS_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	p_driver_obj->MajorFunction[IRP_MJ_PNP] = cl_pnp;
 	p_driver_obj->MajorFunction[IRP_MJ_POWER] = cl_power;
 //	p_driver_obj->MajorFunction[IRP_MJ_DEVICE_CONTROL] = alts_ioctl;
@@ -170,6 +179,8 @@
 
 	UNUSED_PARAM( p_driver_obj );
 
+	CL_DEINIT;
+
 	ALTS_EXIT( ALTS_DBG_DEV );
 }
 
Index: core/complib/kernel/SOURCES
===================================================================
--- core/complib/kernel/SOURCES	(revision 36)
+++ core/complib/kernel/SOURCES	(working copy)
@@ -1,6 +1,6 @@
 TARGETNAME=complib
 TARGETPATH=..\..\..\bin\kernel\obj$(BUILD_ALT_DIR)
-TARGETTYPE=EXPORT_DRIVER
+TARGETTYPE=DRIVER_LIBRARY
 
 DLLDEF=cl_exports.def
 
Index: core/complib/kernel/cl_driver.c
===================================================================
--- core/complib/kernel/cl_driver.c	(revision 36)
+++ core/complib/kernel/cl_driver.c	(working copy)
@@ -33,43 +33,45 @@
 #include "complib/comp_lib.h"
 
 
-NTSTATUS 
-DriverEntry( 
-	IN				DRIVER_OBJECT
*pDevObj, 
-    IN				UNICODE_STRING
*RegistryPath )
-{
-	UNUSED_PARAM( pDevObj );
-	UNUSED_PARAM( RegistryPath );
-	return STATUS_SUCCESS;
-}
+//
+//
+//NTSTATUS 
+//DriverEntry( 
+//	IN				DRIVER_OBJECT
*pDevObj, 
+//    IN				UNICODE_STRING
*RegistryPath )
+//{
+//	UNUSED_PARAM( pDevObj );
+//	UNUSED_PARAM( RegistryPath );
+//	return STATUS_SUCCESS;
+//}
+//
+//
+//NTSTATUS
+//DllInitialize(
+//	IN				UNICODE_STRING
*RegistryPath )
+//{
+//	cl_status_t		status;
+//	UNUSED_PARAM( RegistryPath );
+//
+//	__cl_mem_track( TRUE );
+//
+//	status = cl_obj_mgr_create();
+//
+//	return cl_to_ntstatus( status );
+//}
+//
+//
+//NTSTATUS
+//DllUnload(void)
+//{
+//	cl_obj_mgr_destroy();
+//
+//	__cl_mem_track( FALSE );
+//
+//	return STATUS_SUCCESS;
+//}
 
 
-NTSTATUS
-DllInitialize(
-	IN				UNICODE_STRING
*RegistryPath )
-{
-	cl_status_t		status;
-	UNUSED_PARAM( RegistryPath );
-
-	__cl_mem_track( TRUE );
-
-	status = cl_obj_mgr_create();
-
-	return cl_to_ntstatus( status );
-}
-
-
-NTSTATUS
-DllUnload(void)
-{
-	cl_obj_mgr_destroy();
-
-	__cl_mem_track( FALSE );
-
-	return STATUS_SUCCESS;
-}
-
-
 CL_EXPORT NTSTATUS
 cl_to_ntstatus(
 	IN	cl_status_t	status )
Index: core/complib/cl_memory.c
===================================================================
--- core/complib/cl_memory.c	(revision 36)
+++ core/complib/cl_memory.c	(working copy)
@@ -42,9 +42,11 @@
 
 
 #include "cl_memtrack.h"
+#include <complib/cl_atomic.h>
 
 
 cl_mem_tracker_t		*gp_mem_tracker = NULL;
+atomic32_t				g_mem_trk_ref = 0;
 
 
 /*
@@ -73,7 +75,10 @@
 	cl_status_t			status;
 
 	if( gp_mem_tracker )
+	{
+		cl_atomic_inc( &g_mem_trk_ref );
 		return;
+	}
 
 	/* Allocate the memory tracker object. */
 	gp_mem_tracker = (cl_mem_tracker_t*)
@@ -112,6 +117,9 @@
 	if( !gp_mem_tracker )
 		return;
 
+	if( cl_atomic_dec( &g_mem_trk_ref ) )
+		return;
+
 	if( cl_qmap_count( &gp_mem_tracker->alloc_map ) )
 	{
 		/* There are still items in the list.  Print them out. */
Index: core/complib/cl_obj.c
===================================================================
--- core/complib/cl_obj.c	(revision 36)
+++ core/complib/cl_obj.c	(working copy)
@@ -43,6 +43,7 @@
 
 /* The global object manager. */
 cl_obj_mgr_t				*gp_obj_mgr = NULL;
+atomic32_t					g_cl_obj_ref = 0;
 
 
 
@@ -57,7 +58,10 @@
 
 	/* See if the object manager has already been created. */
 	if( gp_obj_mgr )
+	{
+		cl_atomic_inc( &g_cl_obj_ref );
 		return CL_SUCCESS;
+	}
 
 	/* Allocate the object manager. */
 	gp_obj_mgr = cl_zalloc( sizeof( cl_obj_mgr_t ) );
@@ -70,6 +74,8 @@
 	cl_async_proc_construct( &gp_obj_mgr->async_proc_mgr );
 	cl_qpool_construct( &gp_obj_mgr->rel_pool );
 
+	cl_atomic_inc( &g_cl_obj_ref );
+
 	/* Initialize the spinlock. */
 	status = cl_spinlock_init( &gp_obj_mgr->lock );
 	if( status != CL_SUCCESS )
@@ -110,6 +116,10 @@
 	if( !gp_obj_mgr )
 		return;
 
+	/* See if this is the last call. */
+	if( cl_atomic_dec( &g_cl_obj_ref ) )
+		return;
+
 	/* Verify that all object's have been destroyed. */
 	for( p_list_item = cl_qlist_head( &gp_obj_mgr->obj_list );
 		 p_list_item != cl_qlist_end( &gp_obj_mgr->obj_list );
Index: core/bus/kernel/bus_driver.c
===================================================================
--- core/bus/kernel/bus_driver.c	(revision 36)
+++ core/bus/kernel/bus_driver.c	(working copy)
@@ -327,6 +327,8 @@
 
 	UNUSED_PARAM( p_driver_obj );
 
+	CL_DEINIT;
+
 	BUS_EXIT( BUS_DBG_DRV );
 }
 
@@ -340,6 +342,14 @@
 
 	BUS_ENTER( BUS_DBG_DRV );
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		BUS_TRACE_EXIT( BUS_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	/* Store the driver object pointer in the global parameters. */
 	bus_globals.p_driver_obj = p_driver_obj;
 
@@ -347,6 +357,7 @@
 	status = __read_registry( p_registry_path );
 	if( !NT_SUCCESS(status) )
 	{
+		CL_DEINIT;
 		BUS_TRACE_EXIT( BUS_DBG_ERROR, 
 			("__read_registry returned %08x.\n", status) );
 		return status;
Index: core/al/kernel/al_driver.c
===================================================================
--- core/al/kernel/al_driver.c	(revision 36)
+++ core/al/kernel/al_driver.c	(working copy)
@@ -239,10 +239,19 @@
 
 	AL_ENTER( AL_DBG_DEV );
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		AL_TRACE_EXIT( AL_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	/* Get the registry values. */
 	status = __read_registry( p_registry_path );
 	if( !NT_SUCCESS(status) )
 	{
+		CL_DEINIT;
 		AL_TRACE_EXIT( AL_DBG_ERROR, 
 			("__read_registry returned %08x.\n", status) );
 		return status;
@@ -258,6 +267,8 @@
 {
 	AL_ENTER( AL_DBG_DEV );
 
+	CL_DEINIT;
+
 	AL_EXIT( AL_DBG_DEV );
 	return STATUS_SUCCESS;
 }
Index: core/iou/kernel/iou_driver.c
===================================================================
--- core/iou/kernel/iou_driver.c	(revision 36)
+++ core/iou/kernel/iou_driver.c	(working copy)
@@ -188,6 +188,8 @@
 
 	UNUSED_PARAM( p_driver_obj );
 
+	CL_DEINIT;
+
 	IOU_EXIT( IOU_DBG_DRV );
 }
 
@@ -201,6 +203,14 @@
 
 	IOU_ENTER( IOU_DBG_DRV );
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		IOU_TRACE_EXIT( IOU_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	/* Store the driver object pointer in the global parameters. */
 	iou_globals.p_driver_obj = p_driver_obj;
 
@@ -208,6 +218,7 @@
 	status = __read_registry( p_registry_path );
 	if( !NT_SUCCESS(status) )
 	{
+		CL_DEINIT;
 		IOU_TRACE_EXIT( IOU_DBG_ERROR, 
 			("__read_registry returned %08x.\n", status) );
 		return status;
@@ -220,6 +231,8 @@
 	p_driver_obj->DriverUnload = iou_unload;
 	p_driver_obj->DriverExtension->AddDevice = iou_add_device;
 
+	status = cl_to_ntstatus( cl_obj_mgr_create() );
+
 	IOU_EXIT( IOU_DBG_DRV );
-	return STATUS_SUCCESS;
+	return status;
 }
Index: inc/kernel/complib/cl_types_osd.h
===================================================================
--- inc/kernel/complib/cl_types_osd.h	(revision 36)
+++ inc/kernel/complib/cl_types_osd.h	(working copy)
@@ -46,6 +46,9 @@
 #include <wdmwarn4.h>
 #if defined( NDIS_MINIPORT_DRIVER )
 #include <ndis.h>
+#if NDIS_WDM
+#define CL_NTDDK
+#endif /* NDIS_WDM */
 #elif !defined( _MINIPORT_ )
 #include <ntddk.h>
 #define CL_NTDDK
@@ -93,11 +96,7 @@
 #define UNUSED_PARAM	UNREFERENCED_PARAMETER
 
 
-#if !defined(EXPORT_CL_SYMBOLS)
-#define CL_EXPORT		DECLSPEC_IMPORT
-#else
-#define CL_EXPORT		__declspec(dllexport)
-#endif
+#define CL_EXPORT
 
 #if !defined( __cplusplus )
 #define inline			__inline
@@ -119,15 +118,33 @@
 
 #define CL_CONST64( x )	x##ui64
 
-CL_INLINE NTSTATUS
+NTSTATUS
 cl_to_ntstatus(
 	IN	enum _cl_status	status );
 
-CL_INLINE enum _cl_status
+enum _cl_status
 cl_from_ntstatus(
 	IN	NTSTATUS status );
 
 
+#ifdef MEM_TRACK_ON
+#ifdef NEED_CL_OBJ
+#define CL_INIT		__cl_mem_track( TRUE ), cl_obj_mgr_create()
+#define CL_DEINIT	cl_obj_mgr_destroy(), __cl_mem_track( FALSE )
+#else
+#endif	/* NEED_CL_OBJ */
+#define CL_INIT		__cl_mem_track( TRUE ), STATUS_SUCCESS
+#define CL_DEINIT	__cl_mem_track( FALSE )
+#else	/* MEM_TRACK_ON */
+#ifdef NEED_CL_OBJ
+#define CL_INIT		cl_obj_mgr_create()
+#define CL_DEINIT	cl_obj_mgr_destroy()
+#else
+#define CL_INIT		STATUS_SUCCESS
+#define CL_DEINIT
+#endif	/* NEED_CL_OBJ */
+#endif	/* MEM_TRACK_ON */
+
 #ifdef __cplusplus
 }	/* extern "C" */
 #endif
Index: hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/Md.c
===================================================================
--- hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/Md.c	(revision 36)
+++ hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/Md.c	(working copy)
@@ -255,6 +255,10 @@
 	/* no context yet */
 	g_pDrvContext = NULL;
 	
+	l_Status = CL_INIT;
+	if( !NT_SUCCESS(l_Status) )
+		return l_Status;
+
 	/* create Control Device names */
 	if (!MdCreateDeviceNames(MD_CTL_DEVICE_NAME, &l_usNtDeviceName,
&l_usDosDeviceName))
 	{ /* failed - no resources */
@@ -410,6 +414,8 @@
 	MdKdPrint( DBGLVL_ALWAYS ,("(MdDeviceInit) Device failed to
initialize
\n"));
 #pragma warning( pop )
 
+	CL_DEINIT;
+
 	/* Write to event log */
 	WriteEventLogEntry(	pi_pDriverObject, MD_EVENT_LOG_LOAD_ERROR,
 			0, l_Status, 1, l_Status );
@@ -565,6 +571,9 @@
 
 	/* free the driver context */
 	MdExFreePool( l_pDrvContext );
+
+	CL_DEINIT;
+
 	g_pDrvContext = NULL;
 	
 	MDASSERT( g_pDbgData->m_nExAllocCount == 0 );
Index: hw/mt23108/kernel/hca_driver.c
===================================================================
--- hw/mt23108/kernel/hca_driver.c	(revision 36)
+++ hw/mt23108/kernel/hca_driver.c	(working copy)
@@ -267,9 +267,18 @@
 
 	HCA_ENTER( HCA_DBG_DEV );
 
+	status = CL_INIT;
+	if( !NT_SUCCESS(status) )
+	{
+		HCA_TRACE_EXIT( HCA_DBG_ERROR,
+			("cl_init returned %08X.\n", status) );
+		return status;
+	}
+
 	status = __read_registry( p_registry_path );
 	if( !NT_SUCCESS( status ) )
 	{
+		CL_DEINIT;
 		HCA_TRACE_EXIT( HCA_DBG_ERROR,
 			("__read_registry_path returned 0x%X.\n", status) );
 		return status;
@@ -279,6 +288,7 @@
 	cl_status = mlnx_hobs_init();
 	if( cl_status != CL_SUCCESS )
 	{
+		CL_DEINIT;
 		HCA_TRACE_EXIT( HCA_DBG_ERROR,
 			("mlnx_hobs_init returned %s.\n",
cl_status_text[cl_status]) );
 		return cl_to_ntstatus( cl_status );
@@ -353,6 +363,8 @@
 
 	UNUSED_PARAM( p_driver_obj );
 
+	CL_DEINIT;
+
 	HCA_EXIT( HCA_DBG_DEV );
 }


_______________________________________________
openib-windows mailing list
openib-windows at openib.org
http://openib.org/mailman/listinfo/openib-windows
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20050713/cec47289/attachment.html>


More information about the ofw mailing list