Hi, On Sat, Oct 29, 2011 at 21:25, Dominique Martinet <asmadeus@xxxxxxxxxxxxx> wrote: > Hi, > > > Arend van Spriel wrote on Sat, Oct 29, 2011: >> Though it's unlikely, di may be null, so we can't dereference >> di->dma.dmactrlflags until we've checked it. > > I've got no experience whatsoever in kernel coding so feel free to tell > me that's stupid, but if you say it's unlikely why not use the > unlikely() macro as well for the check? I'm not sure the check for whether di is NULL is necessary at all, as my very quick check of where this function is called from turned up only one place that it's called from, where di is most definitely not null. As for using unlikely() it produces an instruction for the compiler to mangle the if statement into assembler in a certain way and, as I understand it, is mostly used where we *know* that: 1. The test is unlikely to be true and 2. Telling the compiler to write the assembly code like this will produce a performance improvement. I.e. a good place to stick an unlikely is in a "fast-path", e.g. the packet receive code - if we don't process packets fast enough, the hardware will start dropping them, so a performance improvement of a couple of cycles - which is what unlikely() will give us - may visibly improve performance. That said, there are a couple of reasons why we shouldn't: - Compilers are getting smarter all the time, and can probably figure out that we're unlikely to hit the test often - Premature optimisation is considered harmful. - If, as I suspect, this function is only used in one location, the compiler will probably inline the function, then optimise it from there - the only caller I could find calls the function with a proper value for di, so the compiler will probably drop the entire if statement. I'm sure that there are many other things that could be worked on in this driver before we start worrying about whether we should stick unlikely()s in the code. > (also, that's not the point of this patch, but can't hurt to point that > out) This is what I spend my life doing - this patch was just another instance of this =) Thanks, -- Julian Calaby Email: julian.calaby@xxxxxxxxx Profile: http://www.google.com/profiles/julian.calaby/ .Plan: http://sites.google.com/site/juliancalaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
