Applied on 2949. Thanks Tzachi
> -----Original Message----- > From: Smith, Stan [mailto:[email protected]] > Sent: Tuesday, September 21, 2010 7:48 PM > To: Uri Habusha; Fab Tillier; Tzachi Dar; Hefty, Sean; > [email protected] > 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: [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 _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
