Fab I agree with you that in optimal life we should write more tests, run more scenarios on different machine architectures and configurations; however we all know that life is not optimal and many times you have to make tradeoffs.
In this case we added an assert and if it happens on our test environment I promise you to look and investigate it. But we don't want the customer system crashing when that happens; We prefer the application will fail silently. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Fab Tillier Sent: Monday, September 20, 2010 6:14 PM To: Tzachi Dar; Hefty, Sean; [email protected] Subject: Re: [ofw] Patch: Replace memory allocator with a memory allocator that works natively with 0 bytes allocations 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: [email protected] [mailto:[email protected]] On Behalf Of Tzachi Dar Sent: Monday, September 20, 2010 4:25 AM To: Hefty, Sean; [email protected] 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:[email protected]] > Sent: Tuesday, August 31, 2010 7:31 PM > To: Tzachi Dar; [email protected] > 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 [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
