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
