* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) [not found] <200606231502.k5NF2jfO007109@hera.kernel.org> @ 2006-06-23 18:51 ` Jeff Garzik 2006-06-23 21:24 ` Andrew Morton 0 siblings, 1 reply; 15+ messages in thread From: Jeff Garzik @ 2006-06-23 18:51 UTC (permalink / raw) To: Linux Kernel Mailing List, Andrew Morton; +Cc: Linus Torvalds Linux Kernel Mailing List wrote: > commit e6022603b9aa7d61d20b392e69edcdbbc1789969 > tree f94b059e312ea7b2f3c4d0b01939e868ed525fb0 > parent 304c4c841a31c780a45d65e389b07706babf5d36 > author Andrew Morton <akpm@osdl.org> Fri, 23 Jun 2006 16:05:32 -0700 > committer Linus Torvalds <torvalds@g5.osdl.org> Fri, 23 Jun 2006 21:43:05 -0700 > > [PATCH] ext3_clear_inode(): avoid kfree(NULL) > > Steven Rostedt <rostedt@goodmis.org> points out that `rsv' here is usually > NULL, so we should avoid calling kfree(). > > Also, fix up some nearby whitespace damage. > > Signed-off-by: Andrew Morton <akpm@osdl.org> > Signed-off-by: Linus Torvalds <torvalds@osdl.org> Dumb question... why? kfree(NULL) has always been explicitly allowed. Jeff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-23 18:51 ` [PATCH] ext3_clear_inode(): avoid kfree(NULL) Jeff Garzik @ 2006-06-23 21:24 ` Andrew Morton 2006-06-24 12:11 ` Arjan van de Ven 0 siblings, 1 reply; 15+ messages in thread From: Andrew Morton @ 2006-06-23 21:24 UTC (permalink / raw) To: Jeff Garzik; +Cc: linux-kernel, torvalds On Fri, 23 Jun 2006 14:51:03 -0400 Jeff Garzik <jeff@garzik.org> wrote: > Linux Kernel Mailing List wrote: > > commit e6022603b9aa7d61d20b392e69edcdbbc1789969 > > tree f94b059e312ea7b2f3c4d0b01939e868ed525fb0 > > parent 304c4c841a31c780a45d65e389b07706babf5d36 > > author Andrew Morton <akpm@osdl.org> Fri, 23 Jun 2006 16:05:32 -0700 > > committer Linus Torvalds <torvalds@g5.osdl.org> Fri, 23 Jun 2006 21:43:05 -0700 > > > > [PATCH] ext3_clear_inode(): avoid kfree(NULL) > > > > Steven Rostedt <rostedt@goodmis.org> points out that `rsv' here is usually > > NULL, so we should avoid calling kfree(). > > > > Also, fix up some nearby whitespace damage. > > > > Signed-off-by: Andrew Morton <akpm@osdl.org> > > Signed-off-by: Linus Torvalds <torvalds@osdl.org> > > Dumb question... why? kfree(NULL) has always been explicitly allowed. > Because at that callsite, NULL is the common case. We avoid a do-nothing function call most of the time. It's a nano-optimisation. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-23 21:24 ` Andrew Morton @ 2006-06-24 12:11 ` Arjan van de Ven 2006-06-24 12:20 ` Steven Rostedt 2006-06-24 16:19 ` Pekka Enberg 0 siblings, 2 replies; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 12:11 UTC (permalink / raw) To: Andrew Morton; +Cc: Jeff Garzik, linux-kernel, torvalds > > Because at that callsite, NULL is the common case. We avoid a do-nothing > function call most of the time. It's a nano-optimisation. but a function call is basically free, while an if () is not... even with unlikely()... sounds like a misoptimization to me. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:11 ` Arjan van de Ven @ 2006-06-24 12:20 ` Steven Rostedt 2006-06-24 12:27 ` Arjan van de Ven 2006-06-24 16:19 ` Pekka Enberg 1 sibling, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2006-06-24 12:20 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > function call most of the time. It's a nano-optimisation. > > but a function call is basically free, while an if () is not... even > with unlikely()... > > sounds like a misoptimization to me. > How is a function call free when an if is not? Especially if that function does the exact same if? The problem is that in these cases the pointer is NULL several thousands of times for every time it is not NULL (if ever). The non-NULL case is where an error occurred or something very special. So I don't see how the if here is a problem? -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:20 ` Steven Rostedt @ 2006-06-24 12:27 ` Arjan van de Ven 2006-06-24 12:33 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 12:27 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 2006-06-24 at 08:20 -0400, Steven Rostedt wrote: > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > > function call most of the time. It's a nano-optimisation. > > > > but a function call is basically free, while an if () is not... even > > with unlikely()... > > > > sounds like a misoptimization to me. > > > > How is a function call free when an if is not? in general, a function call is 100% predictable without any real control flow dependencies for the processor, and thus there is no real issue in the execution pipeline. An if is a conditional branch, which breaks up the execution pipeline if mispredicted... > Especially if that > function does the exact same if? sure; but to call this code an optimization ... it's just extra code. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:27 ` Arjan van de Ven @ 2006-06-24 12:33 ` Steven Rostedt 2006-06-24 12:46 ` Arjan van de Ven 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2006-06-24 12:33 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 24 Jun 2006, Arjan van de Ven wrote: > On Sat, 2006-06-24 at 08:20 -0400, Steven Rostedt wrote: > > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > > > > > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > > > function call most of the time. It's a nano-optimisation. > > > > > > but a function call is basically free, while an if () is not... even > > > with unlikely()... > > > > > > sounds like a misoptimization to me. > > > > > > > How is a function call free when an if is not? > > in general, a function call is 100% predictable without any real control > flow dependencies for the processor, and thus there is no real issue in > the execution pipeline. An if is a conditional branch, which breaks up > the execution pipeline if mispredicted... But doesn't the unlikely help the prediction? Like I stated, the if may never succeed. -- Steve > > > Especially if that > > function does the exact same if? > > sure; > > but to call this code an optimization ... it's just extra code. > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:33 ` Steven Rostedt @ 2006-06-24 12:46 ` Arjan van de Ven 2006-06-24 12:51 ` Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 12:46 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 2006-06-24 at 08:33 -0400, Steven Rostedt wrote: > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > On Sat, 2006-06-24 at 08:20 -0400, Steven Rostedt wrote: > > > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > > > > > > > > > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > > > > function call most of the time. It's a nano-optimisation. > > > > > > > > but a function call is basically free, while an if () is not... even > > > > with unlikely()... > > > > > > > > sounds like a misoptimization to me. > > > > > > > > > > How is a function call free when an if is not? > > > > in general, a function call is 100% predictable without any real control > > flow dependencies for the processor, and thus there is no real issue in > > the execution pipeline. An if is a conditional branch, which breaks up > > the execution pipeline if mispredicted... > > But doesn't the unlikely help the prediction? nope none at all, at least not on x86/x86-64. (in fact there is no way to help the prediction on those architectures that actually works) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:46 ` Arjan van de Ven @ 2006-06-24 12:51 ` Steven Rostedt 2006-06-24 12:53 ` Arjan van de Ven 2006-06-24 16:49 ` Jeremy Fitzhardinge 2 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2006-06-24 12:51 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > in general, a function call is 100% predictable without any real control > > > flow dependencies for the processor, and thus there is no real issue in > > > the execution pipeline. An if is a conditional branch, which breaks up > > > the execution pipeline if mispredicted... > > > > But doesn't the unlikely help the prediction? > > nope none at all, at least not on x86/x86-64. > (in fact there is no way to help the prediction on those architectures > that actually works) > But doesn't gcc optimize it. That is, it puts the code in the if block as the jump. So if you never take the if, the pipeline is not hurt. -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:46 ` Arjan van de Ven 2006-06-24 12:51 ` Steven Rostedt @ 2006-06-24 12:53 ` Arjan van de Ven 2006-06-24 13:07 ` Steven Rostedt 2006-06-24 16:49 ` Jeremy Fitzhardinge 2 siblings, 1 reply; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 12:53 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 2006-06-24 at 14:46 +0200, Arjan van de Ven wrote: > On Sat, 2006-06-24 at 08:33 -0400, Steven Rostedt wrote: > > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > On Sat, 2006-06-24 at 08:20 -0400, Steven Rostedt wrote: > > > > On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > > > > > > > > > > > > > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > > > > > function call most of the time. It's a nano-optimisation. > > > > > > > > > > but a function call is basically free, while an if () is not... even > > > > > with unlikely()... > > > > > > > > > > sounds like a misoptimization to me. > > > > > > > > > > > > > How is a function call free when an if is not? > > > > > > in general, a function call is 100% predictable without any real control > > > flow dependencies for the processor, and thus there is no real issue in > > > the execution pipeline. An if is a conditional branch, which breaks up > > > the execution pipeline if mispredicted... > > > > But doesn't the unlikely help the prediction? > > nope none at all, at least not on x86/x86-64. > (in fact there is no way to help the prediction on those architectures > that actually works) ok that's not entirely accurate, there's some basic heuristics you can tweak to; eg often the assumption is "if you jump backwards the branch is taken" and stuff like that, but that's generally useful in loops, not so much in if () statements.... since for if () both your branches are forward. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:53 ` Arjan van de Ven @ 2006-06-24 13:07 ` Steven Rostedt 2006-06-24 14:11 ` Arjan van de Ven 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2006-06-24 13:07 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 24 Jun 2006, Arjan van de Ven wrote: > > > > > > But doesn't the unlikely help the prediction? > > > > nope none at all, at least not on x86/x86-64. > > (in fact there is no way to help the prediction on those architectures > > that actually works) > > ok that's not entirely accurate, there's some basic heuristics you can > tweak to; eg often the assumption is "if you jump backwards the branch > is taken" and stuff like that, but that's generally useful in loops, not > so much in if () statements.... since for if () both your branches are > forward. > I just tried this code: --- code --- extern void marker1(void); extern void marker2(void); extern void callme(void *p); #define unlikely(x) __builtin_expect(!!(x), 0) void predictme(void *t) { marker1(); if (unlikely (t)) callme(t); marker2(); return; } --- end code --- Compiled it like: $ gcc -O2 -c predict.c Then did objdump and got this: --- objdump --- 00000000 <predictme>: 0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 53 push %ebx 4: 83 ec 04 sub $0x4,%esp 7: 8b 5d 08 mov 0x8(%ebp),%ebx a: e8 fc ff ff ff call b <predictme+0xb> b: R_386_PC32 marker1 f: 85 db test %ebx,%ebx 11: 75 08 jne 1b <predictme+0x1b> 13: 58 pop %eax 14: 5b pop %ebx 15: 5d pop %ebp 16: e9 fc ff ff ff jmp 17 <predictme+0x17> 17: R_386_PC32 marker2 1b: 89 1c 24 mov %ebx,(%esp) 1e: e8 fc ff ff ff call 1f <predictme+0x1f> 1f: R_386_PC32 callme 23: eb ee jmp 13 <predictme+0x13> --- end objdump -- The jne is expected to fail, so we will always continue to 0x13. Now is this a problem with x86/x86_64? BTW: taking out the unlikely I get this: --- objdump --- 00000000 <predictme>: 0: 55 push %ebp 1: 89 e5 mov %esp,%ebp 3: 53 push %ebx 4: 83 ec 04 sub $0x4,%esp 7: 8b 5d 08 mov 0x8(%ebp),%ebx a: e8 fc ff ff ff call b <predictme+0xb> b: R_386_PC32 marker1 f: 85 db test %ebx,%ebx 11: 74 08 je 1b <predictme+0x1b> 13: 89 1c 24 mov %ebx,(%esp) 16: e8 fc ff ff ff call 17 <predictme+0x17> 17: R_386_PC32 callme 1b: 58 pop %eax 1c: 5b pop %ebx 1d: 5d pop %ebp 1e: e9 fc ff ff ff jmp 1f <predictme+0x1f> 1f: R_386_PC32 marker2 --- end objdump --- So it seems that gcc tries to keep the likely's from jumping. -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 13:07 ` Steven Rostedt @ 2006-06-24 14:11 ` Arjan van de Ven 2006-06-24 16:06 ` Nick Piggin 0 siblings, 1 reply; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 14:11 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds > > The jne is expected to fail, so we will always continue to 0x13. Now is > this a problem with x86/x86_64? I'm not saying there is a problem; likely/unlikely do have an effect for sure, it's just not a "make it free" thing.... ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 14:11 ` Arjan van de Ven @ 2006-06-24 16:06 ` Nick Piggin 0 siblings, 0 replies; 15+ messages in thread From: Nick Piggin @ 2006-06-24 16:06 UTC (permalink / raw) To: Arjan van de Ven Cc: Steven Rostedt, Andrew Morton, Jeff Garzik, linux-kernel, torvalds Arjan van de Ven wrote: >>The jne is expected to fail, so we will always continue to 0x13. Now is >>this a problem with x86/x86_64? > > > I'm not saying there is a problem; likely/unlikely do have an effect for > sure, it's just not a "make it free" thing.... On x86 I think the main saving is the icache one. However I guess gcc would try to align with the branch prediction behaviour for unknown branches too. The microoptimisation is that the call avoids the unlikely branch in kfree. Maybe for x86 this isn't going to matter, but for some architectures in might. -- SUSE Labs, Novell Inc. Send instant messages to your online friends http://au.messenger.yahoo.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:46 ` Arjan van de Ven 2006-06-24 12:51 ` Steven Rostedt 2006-06-24 12:53 ` Arjan van de Ven @ 2006-06-24 16:49 ` Jeremy Fitzhardinge 2006-06-24 16:55 ` Arjan van de Ven 2 siblings, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2006-06-24 16:49 UTC (permalink / raw) To: Arjan van de Ven Cc: Steven Rostedt, Andrew Morton, Jeff Garzik, linux-kernel, torvalds Arjan van de Ven wrote: > nope none at all, at least not on x86/x86-64. > (in fact there is no way to help the prediction on those architectures > that actually works) > The branch prediction hint prefixes (2e & 3e) don't work? J ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 16:49 ` Jeremy Fitzhardinge @ 2006-06-24 16:55 ` Arjan van de Ven 0 siblings, 0 replies; 15+ messages in thread From: Arjan van de Ven @ 2006-06-24 16:55 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: Steven Rostedt, Andrew Morton, Jeff Garzik, linux-kernel, torvalds On Sat, 2006-06-24 at 09:49 -0700, Jeremy Fitzhardinge wrote: > Arjan van de Ven wrote: > > nope none at all, at least not on x86/x86-64. > > (in fact there is no way to help the prediction on those architectures > > that actually works) > > > The branch prediction hint prefixes (2e & 3e) don't work? last I read they are both actively discouraged and ignored by all "current" processors from both AMD and Intel. (see the optimization guide PDF that Intel publishes) Note: this is based on me reading public documentation, not on me reading Intel internal stuff or asking the relevant chip architects, so don't take this as a fully authoritative statement. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ext3_clear_inode(): avoid kfree(NULL) 2006-06-24 12:11 ` Arjan van de Ven 2006-06-24 12:20 ` Steven Rostedt @ 2006-06-24 16:19 ` Pekka Enberg 1 sibling, 0 replies; 15+ messages in thread From: Pekka Enberg @ 2006-06-24 16:19 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Andrew Morton, Jeff Garzik, linux-kernel, torvalds On 6/24/06, Arjan van de Ven <arjan@infradead.org> wrote: > > > > Because at that callsite, NULL is the common case. We avoid a do-nothing > > function call most of the time. It's a nano-optimisation. > > but a function call is basically free, while an if () is not... even > with unlikely()... Yeah, but the called function has an if statement too... Pekka ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2006-06-24 16:56 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <200606231502.k5NF2jfO007109@hera.kernel.org> 2006-06-23 18:51 ` [PATCH] ext3_clear_inode(): avoid kfree(NULL) Jeff Garzik 2006-06-23 21:24 ` Andrew Morton 2006-06-24 12:11 ` Arjan van de Ven 2006-06-24 12:20 ` Steven Rostedt 2006-06-24 12:27 ` Arjan van de Ven 2006-06-24 12:33 ` Steven Rostedt 2006-06-24 12:46 ` Arjan van de Ven 2006-06-24 12:51 ` Steven Rostedt 2006-06-24 12:53 ` Arjan van de Ven 2006-06-24 13:07 ` Steven Rostedt 2006-06-24 14:11 ` Arjan van de Ven 2006-06-24 16:06 ` Nick Piggin 2006-06-24 16:49 ` Jeremy Fitzhardinge 2006-06-24 16:55 ` Arjan van de Ven 2006-06-24 16:19 ` Pekka Enberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).