Messages in this thread | | | Date | Sat, 12 Sep 2009 22:49:29 +0400 | From | Cyrill Gorcunov <> | Subject | Re: [PATCH] x86: apic: convert BUG() to BUG_ON() |
| |
[Daniel Walker - Sat, Sep 12, 2009 at 11:20:41AM -0700] | On Sat, 2009-09-12 at 22:05 +0400, Cyrill Gorcunov wrote: | | > | > Hi Daniel, | > | > I believe having a changelog like | > | > Use short form of "if() BUG()" sequence | > | > would be better perhaps? Since "Coccinelle's BUG_ON semantic patch" | > somehow doesn't describe why it's done. | > | > Don't get me wrong please. It's trivial and seen from patch | > itself _why_ it's done though changelog doesn't say the same. | > | > Perhaps I'm too nagging :) Feel free to ignore me. | | Not nagging, I wondered myself what the benefit was when I ran | Coccinelle. | | For one it condenses duplicate code (i.e. the if()). If the BUG_ON() | macro gets updated with something new, all the users get the updates | automatically. The other thing is your re-using potentially more | advanced code that's inside the macro. In this case it's fairly trivial, | | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0) | | So we're getting the benefit on the new "unlikely" in the apic code. | unlikely/likely calls will usually allow the compiler to create smaller, | and or, more optimized code.
I would not relay on "unlikely" much especially in apic code. Though on some platforms this apic_id_registered returns plain 1 which could (didn't check myself) bring in some optimization (to be fair -- I can't imagine what could be optimized in this particular case, especially since we may have locked operations on mem write).
So I consider it as code shrinking on source level only :)
| | So there are at least two benefits, and I don't see any downside to it. | | Daniel | -- Cyrill
| |