Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/2007, "Andrew Morton" <[EMAIL PROTECTED]> wrote: > Thing is, such a patch would amount to adding a test-for-NULL to codepaths > which we *know* do not need it. There is no point in doing that. Now you're just being stubborn, Andrew ;-). The extra check does not matter much at all for most cases. What it buys us is consistent API with kfree(), more robust slab, and less bugs, no? All the super-hot paths can be fixed with __kmem_cache_free(). As an added bonus, we can remove all extra debugging checks when CONFIG_SLAB_DEBUG is disabled from __kmem_cache_free() as it will be only used in tested, known good code paths so performance for those cases will be even better! So I'll whoop up a patch soonish and send it to you. Perhaps your evil twin will apply it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 March 2007 08:30:27 -0800, Andrew Morton wrote: > On Wed, 21 Mar 2007 16:41:19 +0200 "Pekka Enberg" <[EMAIL PROTECTED]> wrote: > > > Yeah, I'll try to sneak a patch past Andrew. > > That would be sneaky. > > Thing is, such a patch would amount to adding a test-for-NULL to codepaths > which we *know* do not need it. There is no point in doing that. How about two patches, one renaming kmem_cache_free to kmem_cache_free_fast or __kmem_cache_free or whatever pleases you most, the second adding kmem_cache_free with a NULL check. The point is that the easiest way to use kmem_cache_free should be the safest, but not necessarily the fastest. Existing well-tuned and NULL-aware code paths can remain fast, random new code will be safe. Jörn -- Joern's library part 14: http://www.sandpile.org/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 Mar 2007 16:41:19 +0200 "Pekka Enberg" <[EMAIL PROTECTED]> wrote: > On 3/21/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote: > > IMHO one way to find them is to actually slow down kmem_cache_free() and see > > where the performance is hurt. > > Yeah, I'll try to sneak a patch past Andrew. That would be sneaky. Thing is, such a patch would amount to adding a test-for-NULL to codepaths which we *know* do not need it. There is no point in doing that. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, Mar 21, 2007 at 03:36:34PM +0200, Pekka J Enberg wrote: > On Wed, 21 Mar 2007, Jarek Poplawski wrote: > > With __kmem_cache_free you would set #1 I hope, but if > > nobody would use this - debugging time wouldn't change. > > I think you got it backwards. I suggested making the _current_ > kmem_cache_free() deal with NULL (so everyone will get it) and add a new > optimized __kmem_cache_free() for those call-sites that really need it. If you could assure optimized version will be used only with buggy-free code, so you don't waste time for debugging it, then I really got it backwards, sorry! > > On Wed, 21 Mar 2007, Jarek Poplawski wrote: > > This could be acceptable, if there were no problems > > with fixing the errors. But there are problems - bugs > > like this aren't fixed on time - maybe because people > > waste too much time per bug? > > You're barking up the wrong tree here, Jarek. I strongly feel that we > should be more defensive in the slab for the exact reasons you outlined. > There's bunch of bug reports people seem to dismiss as slab errors where > in fact it's caused by a buggy caller. But I can see only one tree here. And I seem to agree with all the rest. So, I probably really got it backwards... Must be going to find the right tree, Jarek P. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote: IMHO one way to find them is to actually slow down kmem_cache_free() and see where the performance is hurt. Yeah, I'll try to sneak a patch past Andrew. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wednesday, 21 March 2007 14:36, Pekka J Enberg wrote: > On Wed, 21 Mar 2007, Jarek Poplawski wrote: > > With __kmem_cache_free you would set #1 I hope, but if > > nobody would use this - debugging time wouldn't change. > > I think you got it backwards. I suggested making the _current_ > kmem_cache_free() deal with NULL (so everyone will get it) and add a new > optimized __kmem_cache_free() for those call-sites that really need it. > > On Wed, 21 Mar 2007, Jarek Poplawski wrote: > > This could be acceptable, if there were no problems > > with fixing the errors. But there are problems - bugs > > like this aren't fixed on time - maybe because people > > waste too much time per bug? > > You're barking up the wrong tree here, Jarek. I strongly feel that we > should be more defensive in the slab for the exact reasons you outlined. > There's bunch of bug reports people seem to dismiss as slab errors where > in fact it's caused by a buggy caller. > > That said, Eric and Andrew make a good point about kmem_cache_free() being > in super-hot paths which clearly must be addressed. The only reason > holding me back is the fact that I don't know what those super-hot > call-sites are (with the exception of network skb allocation) so I am > really in no position to make that patch. IMHO one way to find them is to actually slow down kmem_cache_free() and see where the performance is hurt. Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 Mar 2007, Jarek Poplawski wrote: > With __kmem_cache_free you would set #1 I hope, but if > nobody would use this - debugging time wouldn't change. I think you got it backwards. I suggested making the _current_ kmem_cache_free() deal with NULL (so everyone will get it) and add a new optimized __kmem_cache_free() for those call-sites that really need it. On Wed, 21 Mar 2007, Jarek Poplawski wrote: > This could be acceptable, if there were no problems > with fixing the errors. But there are problems - bugs > like this aren't fixed on time - maybe because people > waste too much time per bug? You're barking up the wrong tree here, Jarek. I strongly feel that we should be more defensive in the slab for the exact reasons you outlined. There's bunch of bug reports people seem to dismiss as slab errors where in fact it's caused by a buggy caller. That said, Eric and Andrew make a good point about kmem_cache_free() being in super-hot paths which clearly must be addressed. The only reason holding me back is the fact that I don't know what those super-hot call-sites are (with the exception of network skb allocation) so I am really in no position to make that patch. Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, Mar 21, 2007 at 02:13:52PM +0200, Pekka Enberg wrote: > On 3/21/07, Jarek Poplawski <[EMAIL PROTECTED]> wrote: > >I think Pekka was right (it looks he changed his mind now) something > >should be done here. I think something like this should be a minimum: > > > >BUG_ON(!objp || virt_to_cache(objp) != cachep); > > > >to show distinctly what's going on. > > No, if we were to add a NULL check in kmem_cache_free(), it should > behave like kfree() does. Anyway, if you feel about this strongly I > suspect the best solution is to add a __kmem_cache_free which does > _not_ have the NULL check and convert those super-hot paths to use it. > Sort of what Andrew suggested already. > Are you sure there is no difference? Would this message below be written? Would you waste youre time to write the patch in this thread? Maybe even repostal of this bug would be unnecessary - because somebody would have seen in a minute something you analyzed at least 0,5h. I don't say it's the best proposal - but at least: 1. we know the rules, 2. we save the diagnosing time for the real problem. With __kmem_cache_free you would set #1 I hope, but if nobody would use this - debugging time wouldn't change. This could be acceptable, if there were no problems with fixing the errors. But there are problems - bugs like this aren't fixed on time - maybe because people waste too much time per bug? If this path is so hot, there is other possibility: - to write a comment about NULLs here, - to require such checks were inserted earlier. Why after this all there is no change in the bio_free? This bio_free still is waiting to pass NULL bi_io_vecs without any warning! Why still no "nr_pages > 0" check in scsi_req_map_sg? Was this patch so obvious - authors weren't so sure (not talking about time)? I think optimizations are good and possible: if there is no bug in some place for 2 or 3 years - then OK. But until there are such bugs - let from 1 driver only - checks should definitely be added, even at a cost of speed. Cheers, Jarek P. On 19-03-2007 09:00, Pekka Enberg wrote: > On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: >> BUG_ON(!PageSlab(page)); >> >> that's seriously screwed up. Do you have CONFIG_DEBUG_SLAB enabled? If >> not, please enable it and retest. > > This is scary. Looking at disassembly of the OOPS: > > Disassembly of section .text: > > <.text>: > 0: 5f pop%edi > 1: c3 ret > 2: 57 push %edi > 3: 89 c1 mov%eax,%ecx > 5: 89 d7 mov%edx,%edi > 7: 8d 92 00 00 00 40 lea0x4000(%edx),%edx > d: 56 push %esi > e: c1 ea 0cshr$0xc,%edx > 11: 53 push %ebx > 12: c1 e2 05shl$0x5,%edx > 15: 03 15 40 5d 5a c0 add0xc05a5d40,%edx > > At this point, edx has the result of virt_to_page(). > > 1b: 8b 02 mov(%edx),%eax > 1d: f6 c4 40test $0x40,%ah > 20: 74 03 je 0x25 > > If it's a compound page, look up the real page from ->private. > > 22: 8b 52 0cmov0xc(%edx),%edx > > Now, reload page flags. > > 25: 8b 02 mov(%edx),%eax > > And test... > > 27: a8 80 test $0x80,%al > 29: 75 04 jne0x2f > 2b: 0f 0b ud2a > 2d: eb fe jmp0x2d > 2f: 39 4a 18cmp%ecx,0x18(%edx) > > [snip, snip] > > EIP is at kmem_cache_free+0x29/0x5a > eax: c180 ebx: f0ae12c0 ecx: c18f73c0 edx: c180 > esi: c1919de0 edi: ebp: 1000 esp: f1fe7e14 > ds: 007b es: 007b ss: 0068 > > But somehow eax and edx have the same value 0xc180 here. Hmm? > > Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/07, Jarek Poplawski <[EMAIL PROTECTED]> wrote: I think Pekka was right (it looks he changed his mind now) something should be done here. I think something like this should be a minimum: BUG_ON(!objp || virt_to_cache(objp) != cachep); to show distinctly what's going on. No, if we were to add a NULL check in kmem_cache_free(), it should behave like kfree() does. Anyway, if you feel about this strongly I suspect the best solution is to add a __kmem_cache_free which does _not_ have the NULL check and convert those super-hot paths to use it. Sort of what Andrew suggested already. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Tue, 20 Mar 2007, Christoph Lameter wrote: > CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to > debug problems. Usually non-slab pages are not passed to kmem_cache_free. Yeah, it's probably not a big deal for kmem_cache_free() but if we make the BUG_ON CONFIG_DEBUG_SLAB, kfree() loses it as well. Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 20-03-2007 08:47, Pekka J Enberg wrote: > On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: ... > On Tue, 20 Mar 2007, Eric Dumazet wrote: >> CPU: AMD64 processors, speed 1992.52 MHz (estimated) >> Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit >> mask of 0x00 (No unit mask) count 10 >> samples %symbol name >> 1861563 4.7882 tg3_start_xmit_dma_bug >> 1375727 3.5386 memcpy_c >> 1166438 3.0002 tcp_v4_rcv >> 1157334 2.9768 kmem_cache_free >> >> In this workload (real server), you can see kmem_cache_free() is number four. > > Thanks for the profile. I still wonder where exactly thouse super-hot > call-sites are... > > On Tue, 20 Mar 2007, Eric Dumazet wrote: >> Adding one test and conditional branch in this super-hot function just to >> correct a bug in a SCSI driver (or whatever) is not *SANE*. Sure! My proposal is to remove a few more - so we could transmit this data from mostly not buggy SCSI drivers really fast! > > Agreed. Unless we can get kmem_cache_free() out of those hot paths, of > course =). > > Pekka IMHO admins care far more about infallibility than speed. Every such "saving" causes the bugs are harder to follow, it takes more time and people to find bug's paths and more bugs goes to stable. I'm sure this admin with SCSI - st problem will next time think 3 times before upgrading to the next "stable" - and it'll be only because of this one buggy driver. (Of course then we don't get his next bug report, either - so the next SCSI bug will be diagnosed at least one kernel version later.) I think Pekka was right (it looks he changed his mind now) something should be done here. I think something like this should be a minimum: BUG_ON(!objp || virt_to_cache(objp) != cachep); to show distinctly what's going on. (BTW, kfree isn't exceptional with NULL treatment - neighbouring slob.c works alike.) Regards, Jarek P. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 20-03-2007 08:47, Pekka J Enberg wrote: On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: ... On Tue, 20 Mar 2007, Eric Dumazet wrote: CPU: AMD64 processors, speed 1992.52 MHz (estimated) Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit mask of 0x00 (No unit mask) count 10 samples %symbol name 1861563 4.7882 tg3_start_xmit_dma_bug 1375727 3.5386 memcpy_c 1166438 3.0002 tcp_v4_rcv 1157334 2.9768 kmem_cache_free In this workload (real server), you can see kmem_cache_free() is number four. Thanks for the profile. I still wonder where exactly thouse super-hot call-sites are... On Tue, 20 Mar 2007, Eric Dumazet wrote: Adding one test and conditional branch in this super-hot function just to correct a bug in a SCSI driver (or whatever) is not *SANE*. Sure! My proposal is to remove a few more - so we could transmit this data from mostly not buggy SCSI drivers really fast! Agreed. Unless we can get kmem_cache_free() out of those hot paths, of course =). Pekka IMHO admins care far more about infallibility than speed. Every such saving causes the bugs are harder to follow, it takes more time and people to find bug's paths and more bugs goes to stable. I'm sure this admin with SCSI - st problem will next time think 3 times before upgrading to the next stable - and it'll be only because of this one buggy driver. (Of course then we don't get his next bug report, either - so the next SCSI bug will be diagnosed at least one kernel version later.) I think Pekka was right (it looks he changed his mind now) something should be done here. I think something like this should be a minimum: BUG_ON(!objp || virt_to_cache(objp) != cachep); to show distinctly what's going on. (BTW, kfree isn't exceptional with NULL treatment - neighbouring slob.c works alike.) Regards, Jarek P. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Tue, 20 Mar 2007, Christoph Lameter wrote: CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to debug problems. Usually non-slab pages are not passed to kmem_cache_free. Yeah, it's probably not a big deal for kmem_cache_free() but if we make the BUG_ON CONFIG_DEBUG_SLAB, kfree() loses it as well. Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/07, Jarek Poplawski [EMAIL PROTECTED] wrote: I think Pekka was right (it looks he changed his mind now) something should be done here. I think something like this should be a minimum: BUG_ON(!objp || virt_to_cache(objp) != cachep); to show distinctly what's going on. No, if we were to add a NULL check in kmem_cache_free(), it should behave like kfree() does. Anyway, if you feel about this strongly I suspect the best solution is to add a __kmem_cache_free which does _not_ have the NULL check and convert those super-hot paths to use it. Sort of what Andrew suggested already. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, Mar 21, 2007 at 02:13:52PM +0200, Pekka Enberg wrote: On 3/21/07, Jarek Poplawski [EMAIL PROTECTED] wrote: I think Pekka was right (it looks he changed his mind now) something should be done here. I think something like this should be a minimum: BUG_ON(!objp || virt_to_cache(objp) != cachep); to show distinctly what's going on. No, if we were to add a NULL check in kmem_cache_free(), it should behave like kfree() does. Anyway, if you feel about this strongly I suspect the best solution is to add a __kmem_cache_free which does _not_ have the NULL check and convert those super-hot paths to use it. Sort of what Andrew suggested already. Are you sure there is no difference? Would this message below be written? Would you waste youre time to write the patch in this thread? Maybe even repostal of this bug would be unnecessary - because somebody would have seen in a minute something you analyzed at least 0,5h. I don't say it's the best proposal - but at least: 1. we know the rules, 2. we save the diagnosing time for the real problem. With __kmem_cache_free you would set #1 I hope, but if nobody would use this - debugging time wouldn't change. This could be acceptable, if there were no problems with fixing the errors. But there are problems - bugs like this aren't fixed on time - maybe because people waste too much time per bug? If this path is so hot, there is other possibility: - to write a comment about NULLs here, - to require such checks were inserted earlier. Why after this all there is no change in the bio_free? This bio_free still is waiting to pass NULL bi_io_vecs without any warning! Why still no nr_pages 0 check in scsi_req_map_sg? Was this patch so obvious - authors weren't so sure (not talking about time)? I think optimizations are good and possible: if there is no bug in some place for 2 or 3 years - then OK. But until there are such bugs - let from 1 driver only - checks should definitely be added, even at a cost of speed. Cheers, Jarek P. On 19-03-2007 09:00, Pekka Enberg wrote: On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: BUG_ON(!PageSlab(page)); that's seriously screwed up. Do you have CONFIG_DEBUG_SLAB enabled? If not, please enable it and retest. This is scary. Looking at disassembly of the OOPS: Disassembly of section .text: .text: 0: 5f pop%edi 1: c3 ret 2: 57 push %edi 3: 89 c1 mov%eax,%ecx 5: 89 d7 mov%edx,%edi 7: 8d 92 00 00 00 40 lea0x4000(%edx),%edx d: 56 push %esi e: c1 ea 0cshr$0xc,%edx 11: 53 push %ebx 12: c1 e2 05shl$0x5,%edx 15: 03 15 40 5d 5a c0 add0xc05a5d40,%edx At this point, edx has the result of virt_to_page(). 1b: 8b 02 mov(%edx),%eax 1d: f6 c4 40test $0x40,%ah 20: 74 03 je 0x25 If it's a compound page, look up the real page from -private. 22: 8b 52 0cmov0xc(%edx),%edx Now, reload page flags. 25: 8b 02 mov(%edx),%eax And test... 27: a8 80 test $0x80,%al 29: 75 04 jne0x2f 2b: 0f 0b ud2a 2d: eb fe jmp0x2d 2f: 39 4a 18cmp%ecx,0x18(%edx) [snip, snip] EIP is at kmem_cache_free+0x29/0x5a eax: c180 ebx: f0ae12c0 ecx: c18f73c0 edx: c180 esi: c1919de0 edi: ebp: 1000 esp: f1fe7e14 ds: 007b es: 007b ss: 0068 But somehow eax and edx have the same value 0xc180 here. Hmm? Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 Mar 2007, Jarek Poplawski wrote: With __kmem_cache_free you would set #1 I hope, but if nobody would use this - debugging time wouldn't change. I think you got it backwards. I suggested making the _current_ kmem_cache_free() deal with NULL (so everyone will get it) and add a new optimized __kmem_cache_free() for those call-sites that really need it. On Wed, 21 Mar 2007, Jarek Poplawski wrote: This could be acceptable, if there were no problems with fixing the errors. But there are problems - bugs like this aren't fixed on time - maybe because people waste too much time per bug? You're barking up the wrong tree here, Jarek. I strongly feel that we should be more defensive in the slab for the exact reasons you outlined. There's bunch of bug reports people seem to dismiss as slab errors where in fact it's caused by a buggy caller. That said, Eric and Andrew make a good point about kmem_cache_free() being in super-hot paths which clearly must be addressed. The only reason holding me back is the fact that I don't know what those super-hot call-sites are (with the exception of network skb allocation) so I am really in no position to make that patch. Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wednesday, 21 March 2007 14:36, Pekka J Enberg wrote: On Wed, 21 Mar 2007, Jarek Poplawski wrote: With __kmem_cache_free you would set #1 I hope, but if nobody would use this - debugging time wouldn't change. I think you got it backwards. I suggested making the _current_ kmem_cache_free() deal with NULL (so everyone will get it) and add a new optimized __kmem_cache_free() for those call-sites that really need it. On Wed, 21 Mar 2007, Jarek Poplawski wrote: This could be acceptable, if there were no problems with fixing the errors. But there are problems - bugs like this aren't fixed on time - maybe because people waste too much time per bug? You're barking up the wrong tree here, Jarek. I strongly feel that we should be more defensive in the slab for the exact reasons you outlined. There's bunch of bug reports people seem to dismiss as slab errors where in fact it's caused by a buggy caller. That said, Eric and Andrew make a good point about kmem_cache_free() being in super-hot paths which clearly must be addressed. The only reason holding me back is the fact that I don't know what those super-hot call-sites are (with the exception of network skb allocation) so I am really in no position to make that patch. IMHO one way to find them is to actually slow down kmem_cache_free() and see where the performance is hurt. Greetings, Rafael - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, Mar 21, 2007 at 03:36:34PM +0200, Pekka J Enberg wrote: On Wed, 21 Mar 2007, Jarek Poplawski wrote: With __kmem_cache_free you would set #1 I hope, but if nobody would use this - debugging time wouldn't change. I think you got it backwards. I suggested making the _current_ kmem_cache_free() deal with NULL (so everyone will get it) and add a new optimized __kmem_cache_free() for those call-sites that really need it. If you could assure optimized version will be used only with buggy-free code, so you don't waste time for debugging it, then I really got it backwards, sorry! On Wed, 21 Mar 2007, Jarek Poplawski wrote: This could be acceptable, if there were no problems with fixing the errors. But there are problems - bugs like this aren't fixed on time - maybe because people waste too much time per bug? You're barking up the wrong tree here, Jarek. I strongly feel that we should be more defensive in the slab for the exact reasons you outlined. There's bunch of bug reports people seem to dismiss as slab errors where in fact it's caused by a buggy caller. But I can see only one tree here. And I seem to agree with all the rest. So, I probably really got it backwards... Must be going to find the right tree, Jarek P. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/07, Rafael J. Wysocki [EMAIL PROTECTED] wrote: IMHO one way to find them is to actually slow down kmem_cache_free() and see where the performance is hurt. Yeah, I'll try to sneak a patch past Andrew. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 Mar 2007 16:41:19 +0200 Pekka Enberg [EMAIL PROTECTED] wrote: On 3/21/07, Rafael J. Wysocki [EMAIL PROTECTED] wrote: IMHO one way to find them is to actually slow down kmem_cache_free() and see where the performance is hurt. Yeah, I'll try to sneak a patch past Andrew. That would be sneaky. Thing is, such a patch would amount to adding a test-for-NULL to codepaths which we *know* do not need it. There is no point in doing that. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Wed, 21 March 2007 08:30:27 -0800, Andrew Morton wrote: On Wed, 21 Mar 2007 16:41:19 +0200 Pekka Enberg [EMAIL PROTECTED] wrote: Yeah, I'll try to sneak a patch past Andrew. That would be sneaky. Thing is, such a patch would amount to adding a test-for-NULL to codepaths which we *know* do not need it. There is no point in doing that. How about two patches, one renaming kmem_cache_free to kmem_cache_free_fast or __kmem_cache_free or whatever pleases you most, the second adding kmem_cache_free with a NULL check. The point is that the easiest way to use kmem_cache_free should be the safest, but not necessarily the fastest. Existing well-tuned and NULL-aware code paths can remain fast, random new code will be safe. Jörn -- Joern's library part 14: http://www.sandpile.org/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/21/2007, Andrew Morton [EMAIL PROTECTED] wrote: Thing is, such a patch would amount to adding a test-for-NULL to codepaths which we *know* do not need it. There is no point in doing that. Now you're just being stubborn, Andrew ;-). The extra check does not matter much at all for most cases. What it buys us is consistent API with kfree(), more robust slab, and less bugs, no? All the super-hot paths can be fixed with __kmem_cache_free(). As an added bonus, we can remove all extra debugging checks when CONFIG_SLAB_DEBUG is disabled from __kmem_cache_free() as it will be only used in tested, known good code paths so performance for those cases will be even better! So I'll whoop up a patch soonish and send it to you. Perhaps your evil twin will apply it. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Tue, 20 Mar 2007, Pekka Enberg wrote: > On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: > > The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB. > > No it shouldn't. Letting non-slab pages pass through causes nasty and > hard to debug problems which is why we have the BUG_ONs in the first > place: CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to debug problems. Usually non-slab pages are not passed to kmem_cache_free. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka J Enberg a écrit : Thanks for the profile. I still wonder where exactly thouse super-hot call-sites are... In this case, it's a typical network server Each time a packet is sent to or received from network, network stack has to allocate/free a skb (kmem_cache_alloc()/kmem_cache_free() and its data (kmalloc/kfree) Other paths are for example dentries allocations, file allocations, ... really many spots for some workloads. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: > > > This is a super-hot path. At some point in time, I wrote: > > Super-hot exactly where? On Tue, 20 Mar 2007, Eric Dumazet wrote: > Don't be silly Pekka ... We have plenty oprofiles results if you dont trust > Andrew. Oh, don't get me wrong, this has certainly nothing to do with "not trusting" Andrew. It's just that "this is a super-hot path" doesn't really help me understand where kmem_cache_free() is so performance sensitive at all. On Tue, 20 Mar 2007, Eric Dumazet wrote: > CPU: AMD64 processors, speed 1992.52 MHz (estimated) > Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit > mask of 0x00 (No unit mask) count 10 > samples %symbol name > 1861563 4.7882 tg3_start_xmit_dma_bug > 1375727 3.5386 memcpy_c > 1166438 3.0002 tcp_v4_rcv > 1157334 2.9768 kmem_cache_free > > In this workload (real server), you can see kmem_cache_free() is number four. Thanks for the profile. I still wonder where exactly thouse super-hot call-sites are... On Tue, 20 Mar 2007, Eric Dumazet wrote: > Adding one test and conditional branch in this super-hot function just to > correct a bug in a SCSI driver (or whatever) is not *SANE*. Agreed. Unless we can get kmem_cache_free() out of those hot paths, of course =). Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka Enberg a écrit : On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: This is a super-hot path. Super-hot exactly where? Don't be silly Pekka ... We have plenty oprofiles results if you dont trust Andrew. CPU: AMD64 processors, speed 1992.52 MHz (estimated) Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit mask of 0x00 (No unit mask) count 10 samples %symbol name 1861563 4.7882 tg3_start_xmit_dma_bug 1375727 3.5386 memcpy_c 1166438 3.0002 tcp_v4_rcv 1157334 2.9768 kmem_cache_free In this workload (real server), you can see kmem_cache_free() is number four. Adding one test and conditional branch in this super-hot function just to correct a bug in a SCSI driver (or whatever) is not *SANE*. Numbers talk. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: This is a super-hot path. Super-hot exactly where? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton <[EMAIL PROTECTED]> wrote: The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB. No it shouldn't. Letting non-slab pages pass through causes nasty and hard to debug problems which is why we have the BUG_ONs in the first place: http://lkml.org/lkml/2006/5/8/101 Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB. No it shouldn't. Letting non-slab pages pass through causes nasty and hard to debug problems which is why we have the BUG_ONs in the first place: http://lkml.org/lkml/2006/5/8/101 Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: This is a super-hot path. Super-hot exactly where? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka Enberg a écrit : On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: This is a super-hot path. Super-hot exactly where? Don't be silly Pekka ... We have plenty oprofiles results if you dont trust Andrew. CPU: AMD64 processors, speed 1992.52 MHz (estimated) Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit mask of 0x00 (No unit mask) count 10 samples %symbol name 1861563 4.7882 tg3_start_xmit_dma_bug 1375727 3.5386 memcpy_c 1166438 3.0002 tcp_v4_rcv 1157334 2.9768 kmem_cache_free In this workload (real server), you can see kmem_cache_free() is number four. Adding one test and conditional branch in this super-hot function just to correct a bug in a SCSI driver (or whatever) is not *SANE*. Numbers talk. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: This is a super-hot path. At some point in time, I wrote: Super-hot exactly where? On Tue, 20 Mar 2007, Eric Dumazet wrote: Don't be silly Pekka ... We have plenty oprofiles results if you dont trust Andrew. Oh, don't get me wrong, this has certainly nothing to do with not trusting Andrew. It's just that this is a super-hot path doesn't really help me understand where kmem_cache_free() is so performance sensitive at all. On Tue, 20 Mar 2007, Eric Dumazet wrote: CPU: AMD64 processors, speed 1992.52 MHz (estimated) Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit mask of 0x00 (No unit mask) count 10 samples %symbol name 1861563 4.7882 tg3_start_xmit_dma_bug 1375727 3.5386 memcpy_c 1166438 3.0002 tcp_v4_rcv 1157334 2.9768 kmem_cache_free In this workload (real server), you can see kmem_cache_free() is number four. Thanks for the profile. I still wonder where exactly thouse super-hot call-sites are... On Tue, 20 Mar 2007, Eric Dumazet wrote: Adding one test and conditional branch in this super-hot function just to correct a bug in a SCSI driver (or whatever) is not *SANE*. Agreed. Unless we can get kmem_cache_free() out of those hot paths, of course =). Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka J Enberg a écrit : Thanks for the profile. I still wonder where exactly thouse super-hot call-sites are... In this case, it's a typical network server Each time a packet is sent to or received from network, network stack has to allocate/free a skb (kmem_cache_alloc()/kmem_cache_free() and its data (kmalloc/kfree) Other paths are for example dentries allocations, file allocations, ... really many spots for some workloads. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Tue, 20 Mar 2007, Pekka Enberg wrote: On 3/19/07, Andrew Morton [EMAIL PROTECTED] wrote: The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB. No it shouldn't. Letting non-slab pages pass through causes nasty and hard to debug problems which is why we have the BUG_ONs in the first place: CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to debug problems. Usually non-slab pages are not passed to kmem_cache_free. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka J Enberg wrote:
> From: Pekka Enberg <[EMAIL PROTECTED]>
>
> This changes kmem_cache_free() to deal with NULL objects passed to it. The
> current behavior is inconsistent with kfree() so there are callers
> passing NULL to kmem_cache_free().
>
> Andreas, can you please confirm this fixes the oops you reported on
> linux-scsi?
>
Didn't test this as Mike Christie pointed me to a working fix for the st
driver.
> Cc: Andreas Steinmetz <[EMAIL PROTECTED]>
> Cc: Christoph Lameter <[EMAIL PROTECTED]>
> Signed-off-by: Pekka Enberg <[EMAIL PROTECTED]>
> ---
> mm/slab.c |5 +
> 1 file changed, 5 insertions(+)
>
> Index: 2.6/mm/slab.c
> ===
> --- 2.6.orig/mm/slab.c2007-03-19 10:18:52.0 +0200
> +++ 2.6/mm/slab.c 2007-03-19 10:19:42.0 +0200
> @@ -3741,6 +3741,8 @@ EXPORT_SYMBOL(__kmalloc);
> * @cachep: The cache the allocation was from.
> * @objp: The previously allocated object.
> *
> + * If @objp is NULL, no operation is performed.
> + *
> * Free an object which was previously allocated from this
> * cache.
> */
> @@ -3748,6 +3750,9 @@ void kmem_cache_free(struct kmem_cache *
> {
> unsigned long flags;
>
> + if (unlikely(!objp))
> + return;
> +
> BUG_ON(virt_to_cache(objp) != cachep);
>
> local_irq_save(flags);
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 March 2007 14:10:38 -0700, Andrew Morton wrote:
>
> Would prefer to do:
>
> static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> void *objp)
> {
> if (objp)
> kmem_cache_free(cachep, objp);
> }
>
> so that we don't add extra overhead to all the thousands of existing,
> well-behaved callsites.
In principle, this would work. But two things need changing, imho:
1. Don't inline the function. kmem_cache_free() has only about 34 NULL
callers, if my grep is reliable, so this case is arguable. But in
general, out-of-line functions are better than many extra
conditionals pulled in through the inline one.
2. Switch the names. According to Rusty's benchmark, the easiest way to
use and interface should be the correct one. Every new driver
written by a rookie will call kmem_cache_free(), simply because the
name seems simpler.
void kmem_cache_free_fast(struct kmem_cache *cachep, void *objp)
{
/* old kmem_cache_free() */
}
void kmem_cache_free(struct kmem_cache *cachep, void *objp)
{
if (likely(objp))
kmem_cache_free_fast(cachep, objp);
}
Jörn
--
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is easily forgotten.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 02:41:00PM -0700, Andrew Morton wrote:
> On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
> "Pekka Enberg" <[EMAIL PROTECTED]> wrote:
>
> >
> > On 3/19/2007, "Andrew Morton" <[EMAIL PROTECTED]> wrote:
> > > Would prefer to do:
> > >
> > > static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> > > void *objp)
> > > {
> > > if (objp)
> > > kmem_cache_free(cachep, objp);
> > > }
> > >
> > > so that we don't add extra overhead to all the thousands of existing,
> > > well-behaved callsites.
> >
> > That bloats kernel text all the same
>
> But only for those callsites which choose to use it! We avoid adding a
> test-and-branch to those thousands of callsite which don't need it.
>
> This is a super-hot path.
You're right about that. Perhaps there's some clever exception
handling thing we can do to eliminate the hot-path overhead.
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 02:16:01PM -0700, Christoph Lameter wrote:
> On Mon, 19 Mar 2007, Matt Mackall wrote:
>
> > I think this sort of thing should work:
> >
> > a = kmalloc(...)
> > b = kmem_cache_alloc(..)
> > c = allocate_some_id(...)
> > if (!a || !b || !c) {
> >free_some_id(c)
> >kmem_cache_free(c)
>
> this requires the specification of a kmem_cache structure and the
> object must be allocated by that cache.
Yes, omitted for brevity.
> >kfree(a);
>
> Here we dynamically determine the slab cache and do not verify even which
> slab it came from.
That's an implementation detail that you shouldn't rely on and that
SLOB in fact breaks. kfree(kmem_cache_alloc(...)) is bad style to the
point of being a bug.
In my opinion:
xxxfree(xxxalloc(...)); /* should always work, even if allocation fails */
yyyfree(xxxalloc(...)); /* should never be expected to work */
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
"Pekka Enberg" <[EMAIL PROTECTED]> wrote:
>
> On 3/19/2007, "Andrew Morton" <[EMAIL PROTECTED]> wrote:
> > Would prefer to do:
> >
> > static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> > void *objp)
> > {
> > if (objp)
> > kmem_cache_free(cachep, objp);
> > }
> >
> > so that we don't add extra overhead to all the thousands of existing,
> > well-behaved callsites.
>
> That bloats kernel text all the same
But only for those callsites which choose to use it! We avoid adding a
test-and-branch to those thousands of callsite which don't need it.
This is a super-hot path.
> so it's much cleaner to just make
> the callers explicitly check for NULL then. That said, I'm sorry but I
> just don't buy the "overhead" part of your argument since it's one
> branch and no extra data cache pressure especially as we're already
> doing the BUG_ON and page flag checking.
The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB.
> But, since you're NAKing my patch, we need to get the mempool for from
> the original thread in to fix the oops.
We need to fix scsi rather than working around it in slab or in mempool -
it appears that it's getting its sg lists tangled up, and the problem has
been known since November (at least).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/2007, "Andrew Morton" <[EMAIL PROTECTED]> wrote:
> Would prefer to do:
>
> static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> void *objp)
> {
> if (objp)
> kmem_cache_free(cachep, objp);
> }
>
> so that we don't add extra overhead to all the thousands of existing,
> well-behaved callsites.
That bloats kernel text all the same so it's much cleaner to just make
the callers explicitly check for NULL then. That said, I'm sorry but I
just don't buy the "overhead" part of your argument since it's one
branch and no extra data cache pressure especially as we're already
doing the BUG_ON and page flag checking.
But, since you're NAKing my patch, we need to get the mempool for from
the original thread in to fix the oops.
Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Matt Mackall wrote:
> I think this sort of thing should work:
>
> a = kmalloc(...)
> b = kmem_cache_alloc(..)
> c = allocate_some_id(...)
> if (!a || !b || !c) {
>free_some_id(c)
>kmem_cache_free(c)
this requires the specification of a kmem_cache structure and the
object must be allocated by that cache.
>kfree(a);
Here we dynamically determine the slab cache and do not verify even which
slab it came from.
So you can always use kfree if you do not care. kmem_cache_free verifies
correctness.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 15:49:46 -0500
Matt Mackall <[EMAIL PROTECTED]> wrote:
> On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
> > On Mon, 19 Mar 2007, Pekka J Enberg wrote:
> >
> > > This changes kmem_cache_free() to deal with NULL objects passed to it.
> > > The
> > > current behavior is inconsistent with kfree() so there are callers
> > > passing NULL to kmem_cache_free().
> >
> > Hmmm.. kmem_cache_free is significantly different. One also needs to
> > specify the slab cache.
>
> I think this sort of thing should work:
>
> a = kmalloc(...)
> b = kmem_cache_alloc(..)
> c = allocate_some_id(...)
> if (!a || !b || !c) {
>free_some_id(c)
>kmem_cache_free(c)
>kfree(a);
>return -ENOMEM;
> }
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
> On Mon, 19 Mar 2007, Pekka J Enberg wrote:
>
> > This changes kmem_cache_free() to deal with NULL objects passed to it. The
> > current behavior is inconsistent with kfree() so there are callers
> > passing NULL to kmem_cache_free().
>
> Hmmm.. kmem_cache_free is significantly different. One also needs to
> specify the slab cache.
I think this sort of thing should work:
a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
free_some_id(c)
kmem_cache_free(c)
kfree(a);
return -ENOMEM;
}
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Christoph Lameter wrote: > Hmmm.. kmem_cache_free is significantly different. One also needs to > specify the slab cache. No, it really isn't. Why would we want kfree() to be special? It's only going to confuse people which results in bugs. On Mon, 19 Mar 2007, Christoph Lameter wrote: > Could we fix the BUG instead? Yes, it should be fixed. But we still have a problem with block layer (and probably others) passing NULL to mempool_free. But where should we fix it if not slab? Is the problem ih bio_alloc_bioset() in fs/bio.c as it's leaving a ->bi_io_vec NULL? Or is it in bio_free() forgetting to check for NULL? Or maybe in mempool_free() in mm/mempool.c? It gets messy real quick because you do need to be able to say "this data is optional." Furthemore, the NULL sematics of kfree() are also useful for error handling. It's much safer to deal with this at slab level so why leave it out? Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Pekka J Enberg wrote: > This changes kmem_cache_free() to deal with NULL objects passed to it. The > current behavior is inconsistent with kfree() so there are callers > passing NULL to kmem_cache_free(). Hmmm.. kmem_cache_free is significantly different. One also needs to specify the slab cache. > Andreas, can you please confirm this fixes the oops you reported on > linux-scsi? Could we fix the BUG instead? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Hi, On Mon, 19 Mar 2007, Andrew Morton wrote: > err, we don't want to do this, do we? It adds overhead for something which > we've carefully taught all our programmers to not do. The only known code > which will benefit from this is buggy. Well, I actually disagree with that. It makes little sense for kmem_cache_free() to behave differently from kfree() especially since the overhead is minimal. But anyway, if you're unhappy with the patch, then we should make it explicit that you're not allowed to pass NULL to kmem_cache_free(), mempool_free() and perhaps others as well in which case kfree() comes even more special... I know this has been discussed in the past but I think you should be able to blindly pass whetever pointer the allocator fuction gives you to the corresponding release function which is why I wanted to fix kmem_cache_free() in the first place. On Mon, 19 Mar 2007, Andrew Morton wrote: > s/fix/hide/ No, even though there is clearly some problem with either the scsi or block layer not allocating any pages for the iovec, I do think slab should deal with NULL pointers properly. Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 10:27:18 +0200 (EET) Pekka J Enberg <[EMAIL PROTECTED]> wrote: > From: Pekka Enberg <[EMAIL PROTECTED]> > > This changes kmem_cache_free() to deal with NULL objects passed to it. The > current behavior is inconsistent with kfree() so there are callers > passing NULL to kmem_cache_free(). err, we don't want to do this, do we? It adds overhead for something which we've carefully taught all our programmers to not do. The only known code which will benefit from this is buggy. > Andreas, can you please confirm this fixes the oops you reported on > linux-scsi? s/fix/hide/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 10:27:18 +0200 (EET) Pekka J Enberg [EMAIL PROTECTED] wrote: From: Pekka Enberg [EMAIL PROTECTED] This changes kmem_cache_free() to deal with NULL objects passed to it. The current behavior is inconsistent with kfree() so there are callers passing NULL to kmem_cache_free(). err, we don't want to do this, do we? It adds overhead for something which we've carefully taught all our programmers to not do. The only known code which will benefit from this is buggy. Andreas, can you please confirm this fixes the oops you reported on linux-scsi? s/fix/hide/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Hi, On Mon, 19 Mar 2007, Andrew Morton wrote: err, we don't want to do this, do we? It adds overhead for something which we've carefully taught all our programmers to not do. The only known code which will benefit from this is buggy. Well, I actually disagree with that. It makes little sense for kmem_cache_free() to behave differently from kfree() especially since the overhead is minimal. But anyway, if you're unhappy with the patch, then we should make it explicit that you're not allowed to pass NULL to kmem_cache_free(), mempool_free() and perhaps others as well in which case kfree() comes even more special... I know this has been discussed in the past but I think you should be able to blindly pass whetever pointer the allocator fuction gives you to the corresponding release function which is why I wanted to fix kmem_cache_free() in the first place. On Mon, 19 Mar 2007, Andrew Morton wrote: s/fix/hide/ No, even though there is clearly some problem with either the scsi or block layer not allocating any pages for the iovec, I do think slab should deal with NULL pointers properly. Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Pekka J Enberg wrote: This changes kmem_cache_free() to deal with NULL objects passed to it. The current behavior is inconsistent with kfree() so there are callers passing NULL to kmem_cache_free(). Hmmm.. kmem_cache_free is significantly different. One also needs to specify the slab cache. Andreas, can you please confirm this fixes the oops you reported on linux-scsi? Could we fix the BUG instead? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Christoph Lameter wrote: Hmmm.. kmem_cache_free is significantly different. One also needs to specify the slab cache. No, it really isn't. Why would we want kfree() to be special? It's only going to confuse people which results in bugs. On Mon, 19 Mar 2007, Christoph Lameter wrote: Could we fix the BUG instead? Yes, it should be fixed. But we still have a problem with block layer (and probably others) passing NULL to mempool_free. But where should we fix it if not slab? Is the problem ih bio_alloc_bioset() in fs/bio.c as it's leaving a -bi_io_vec NULL? Or is it in bio_free() forgetting to check for NULL? Or maybe in mempool_free() in mm/mempool.c? It gets messy real quick because you do need to be able to say this data is optional. Furthemore, the NULL sematics of kfree() are also useful for error handling. It's much safer to deal with this at slab level so why leave it out? Pekka - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
On Mon, 19 Mar 2007, Pekka J Enberg wrote:
This changes kmem_cache_free() to deal with NULL objects passed to it. The
current behavior is inconsistent with kfree() so there are callers
passing NULL to kmem_cache_free().
Hmmm.. kmem_cache_free is significantly different. One also needs to
specify the slab cache.
I think this sort of thing should work:
a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
free_some_id(c)
kmem_cache_free(c)
kfree(a);
return -ENOMEM;
}
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 15:49:46 -0500
Matt Mackall [EMAIL PROTECTED] wrote:
On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
On Mon, 19 Mar 2007, Pekka J Enberg wrote:
This changes kmem_cache_free() to deal with NULL objects passed to it.
The
current behavior is inconsistent with kfree() so there are callers
passing NULL to kmem_cache_free().
Hmmm.. kmem_cache_free is significantly different. One also needs to
specify the slab cache.
I think this sort of thing should work:
a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
free_some_id(c)
kmem_cache_free(c)
kfree(a);
return -ENOMEM;
}
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007, Matt Mackall wrote:
I think this sort of thing should work:
a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
free_some_id(c)
kmem_cache_free(c)
this requires the specification of a kmem_cache structure and the
object must be allocated by that cache.
kfree(a);
Here we dynamically determine the slab cache and do not verify even which
slab it came from.
So you can always use kfree if you do not care. kmem_cache_free verifies
correctness.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On 3/19/2007, Andrew Morton [EMAIL PROTECTED] wrote:
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
That bloats kernel text all the same so it's much cleaner to just make
the callers explicitly check for NULL then. That said, I'm sorry but I
just don't buy the overhead part of your argument since it's one
branch and no extra data cache pressure especially as we're already
doing the BUG_ON and page flag checking.
But, since you're NAKing my patch, we need to get the mempool for from
the original thread in to fix the oops.
Pekka
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
Pekka Enberg [EMAIL PROTECTED] wrote:
On 3/19/2007, Andrew Morton [EMAIL PROTECTED] wrote:
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
That bloats kernel text all the same
But only for those callsites which choose to use it! We avoid adding a
test-and-branch to those thousands of callsite which don't need it.
This is a super-hot path.
so it's much cleaner to just make
the callers explicitly check for NULL then. That said, I'm sorry but I
just don't buy the overhead part of your argument since it's one
branch and no extra data cache pressure especially as we're already
doing the BUG_ON and page flag checking.
The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB.
But, since you're NAKing my patch, we need to get the mempool for from
the original thread in to fix the oops.
We need to fix scsi rather than working around it in slab or in mempool -
it appears that it's getting its sg lists tangled up, and the problem has
been known since November (at least).
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 02:16:01PM -0700, Christoph Lameter wrote:
On Mon, 19 Mar 2007, Matt Mackall wrote:
I think this sort of thing should work:
a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
free_some_id(c)
kmem_cache_free(c)
this requires the specification of a kmem_cache structure and the
object must be allocated by that cache.
Yes, omitted for brevity.
kfree(a);
Here we dynamically determine the slab cache and do not verify even which
slab it came from.
That's an implementation detail that you shouldn't rely on and that
SLOB in fact breaks. kfree(kmem_cache_alloc(...)) is bad style to the
point of being a bug.
In my opinion:
xxxfree(xxxalloc(...)); /* should always work, even if allocation fails */
yyyfree(xxxalloc(...)); /* should never be expected to work */
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, Mar 19, 2007 at 02:41:00PM -0700, Andrew Morton wrote:
On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
Pekka Enberg [EMAIL PROTECTED] wrote:
On 3/19/2007, Andrew Morton [EMAIL PROTECTED] wrote:
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
That bloats kernel text all the same
But only for those callsites which choose to use it! We avoid adding a
test-and-branch to those thousands of callsite which don't need it.
This is a super-hot path.
You're right about that. Perhaps there's some clever exception
handling thing we can do to eliminate the hot-path overhead.
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
On Mon, 19 March 2007 14:10:38 -0700, Andrew Morton wrote:
Would prefer to do:
static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
void *objp)
{
if (objp)
kmem_cache_free(cachep, objp);
}
so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.
In principle, this would work. But two things need changing, imho:
1. Don't inline the function. kmem_cache_free() has only about 34 NULL
callers, if my grep is reliable, so this case is arguable. But in
general, out-of-line functions are better than many extra
conditionals pulled in through the inline one.
2. Switch the names. According to Rusty's benchmark, the easiest way to
use and interface should be the correct one. Every new driver
written by a rookie will call kmem_cache_free(), simply because the
name seems simpler.
void kmem_cache_free_fast(struct kmem_cache *cachep, void *objp)
{
/* old kmem_cache_free() */
}
void kmem_cache_free(struct kmem_cache *cachep, void *objp)
{
if (likely(objp))
kmem_cache_free_fast(cachep, objp);
}
Jörn
--
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is easily forgotten.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free
Pekka J Enberg wrote:
From: Pekka Enberg [EMAIL PROTECTED]
This changes kmem_cache_free() to deal with NULL objects passed to it. The
current behavior is inconsistent with kfree() so there are callers
passing NULL to kmem_cache_free().
Andreas, can you please confirm this fixes the oops you reported on
linux-scsi?
Didn't test this as Mike Christie pointed me to a working fix for the st
driver.
Cc: Andreas Steinmetz [EMAIL PROTECTED]
Cc: Christoph Lameter [EMAIL PROTECTED]
Signed-off-by: Pekka Enberg [EMAIL PROTECTED]
---
mm/slab.c |5 +
1 file changed, 5 insertions(+)
Index: 2.6/mm/slab.c
===
--- 2.6.orig/mm/slab.c2007-03-19 10:18:52.0 +0200
+++ 2.6/mm/slab.c 2007-03-19 10:19:42.0 +0200
@@ -3741,6 +3741,8 @@ EXPORT_SYMBOL(__kmalloc);
* @cachep: The cache the allocation was from.
* @objp: The previously allocated object.
*
+ * If @objp is NULL, no operation is performed.
+ *
* Free an object which was previously allocated from this
* cache.
*/
@@ -3748,6 +3750,9 @@ void kmem_cache_free(struct kmem_cache *
{
unsigned long flags;
+ if (unlikely(!objp))
+ return;
+
BUG_ON(virt_to_cache(objp) != cachep);
local_irq_save(flags);
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
