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

[Tzachi Dar]

Applied on 2949.

Thanks
Tzachi

> -----Original Message-----
> From: Smith, Stan [mailto:stan.smith at intel.com]
> Sent: Tuesday, September 21, 2010 7:48 PM
> To: Uri Habusha; Fab Tillier; 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
> 
> 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