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

[Smith, Stan]

Uri Habusha wrote:
> 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.

At this point I think we can disagree and commit, given the Mellanox commitment to continue to investigate the failure cases.
If this is not acceptable, then we need to embrace a Mellanox specific patch, perhaps an #ifdef MELLANOX ?

Forward progress is the end-goal.

stan.

>
> 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: ofw-bounces at lists.openfabrics.org
> [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Fab Tillier
> Sent: Monday, September 20, 2010 6:14 PM
> To: Tzachi Dar; 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
>
> 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
>
> _______________________________________________
> ofw mailing list
> ofw at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
> _______________________________________________
> ofw mailing list
> ofw at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw