[ofw] Patch: Replace memory allocator with a memory allocator that works natively with 0 bytes allocations

[Fab Tillier]

Hi Tzachi,

This still doesn't solve the issue people (myself included) had with the original patch: you are masking bugs in calling code by delaying the check for zero size and hiding it here.  The assert is fine, but I'd skip the code that returns NULL.  If the assert fires, fix the caller.  If the assert doesn't fire but you have runtime crashes at customer sites, you likely need more thorough test coverage.  In any case, runtime crashes will generate a minidump (potentially reported through Windows Error Reporting, assuming you registered for those drivers in your WinQual account) and will show you the call stack and you can fix the caller.

I'd actually like to see the code move away from all these abstractions (especially since the core code is Windows specific, and the HCA drivers have their own abstraction layer for things that are shared with Linux.)  Replace all calls to cl_malloc/cl_zalloc with the appropriate call to ExAllocatePoolWithTag.

-Fab

-----Original Message-----
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Tzachi Dar
Sent: Monday, September 20, 2010 4:25 AM
To: Hefty, Sean; ofw at lists.openfabrics.org
Subject: Re: [ofw] Patch: Replace memory allocator with a memory allocator that works natively with 0 bytes allocations

Here is a new and much smaller version of the patch that does not introduce new functions or new abstractions. It also only touches a very small number of places.

I hope this time it is OK.

Index: core/complib/kernel/cl_memory_osd.c
===================================================================
--- core/complib/kernel/cl_memory_osd.c	(revision 2933)
+++ core/complib/kernel/cl_memory_osd.c	(working copy)
@@ -38,6 +38,10 @@
 	IN	const size_t	size,
 	IN	const boolean_t	pageable )
 {
+	CL_ASSERT(size != 0);
+	if (size ==0) {
+		return NULL;
+	}
 	if( pageable )
 	{
 		CL_ASSERT( KeGetCurrentIrql() < DISPATCH_LEVEL );
Index: hw/mthca/kernel/mt_memory.h
===================================================================
--- hw/mthca/kernel/mt_memory.h	(revision 2933)
+++ hw/mthca/kernel/mt_memory.h	(working copy)
@@ -50,6 +50,10 @@
 {
 	void *ptr;
 	MT_ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL );
+	MT_ASSERT(bsize);
+	if(bsize == 0) {
+		return NULL;
+	}
 	switch (gfp_mask) {
 		case GFP_ATOMIC:
 			ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_ATOMIC );


Thanks
Tzachi

> -----Original Message-----
> From: Hefty, Sean [mailto:sean.hefty at intel.com]
> Sent: Tuesday, August 31, 2010 7:31 PM
> To: Tzachi Dar; ofw at lists.openfabrics.org
> Subject: RE: [ofw] Patch: Replace memory allocator with a memory
> allocator that works natively with 0 bytes allocations
> 
> There are several unrelated changes in this patch, and I personally
> still don't like the idea.  First, it adds overhead that's not needed
> in most cases.  Second, it uses a function name that looks like a
> Windows call, but isn't.  Someone reading the code has to discover that
> the call is some macro.  And in order to make this change, you already
> have to look at and touch everywhere that the size could be 0.  Why not
> just fix those places already?
> 
> There is already WAY too much abstraction in the code base.  Adding
> more does not fix things, it only makes it worse.  This can end up
> hiding bugs.  Upper level code should not work around problems in other
> code, especially drivers.

_______________________________________________
ofw mailing list
ofw at lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw