On Thu, Apr 4, 2019 at 1:06 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > On Wed, Apr 03, 2019 at 03:55:20PM -0700, Cong Wang wrote: > > I tried checkpatch.pl, it has no such a complain. > > Huh? Huh +1? > > I sorry, I must have been very unclear if you're asking about > checkpatch. Checkpatch is a Perl script. It's basically like grep. It > has no idea about fast paths or benchmarking. Let me try explain > better. Sure, you said coding style, then I brought up checkpatch.pl. If it hurts you, then I am sorry. How do you check your coding style without checkpatch.pl? I am happy to learn. Personally I just trust checkpatch.pl, because I don't want to waste my time on coding styles, I never say I like it or not, I just need a tool. Thanks for teaching me what checkpatch.pl is, although I have been using it for so many years, it is really really helpful. > > The likely/unlikely annotations are there to help optimize branch > prediction and make the code run faster. In the real world if you can't > benchmark or measure or detect a speed improvement then it's not a real > speed improvement. Personally I disagree with you on this point. You don't have to measure, you just have to understand the code. In this case, bluetooth has linear and sane skb's in _normal_ case, you don't disagree, do you? So even _if_ likely()/unlikely() is only for telling which is the normal case, it is still helpful. In this case, we want to tell non-linear skb's are unlikely in bluetooth, you know this is true, as linear is always the case; and, we want to tell ill-formatted skb's are unlikely too, and you know this is also true. > > 1) HCI events don't happen often enough to where the speed can be easily > benchmarked. In that situation, we just write the code as readably > as possible instead of trying to micro optimize it. Same. It is not about benchmarking. > > 2) The compiler already does common sense branch prediction. These > conditions look straight forward so it probably gets it right. You I'd be surprised if the compiler could know skb's are always linear in this particular case. > should take a look at the object code. The compiler probably gets > it right. I bet that these annotations don't even affect the > compiled code let alone the benchmarking output. If you are suggesting we should remove all likely()/unlikely() from pskb_may_pull() callers, please go ahead to submit a patch to remove them all, and see if David accepts it. > > I know this isn't explained in CodingStyle but some things are > assumed. In drivers/ about 1.5% of if statements have likely/unlikely > annotations. It really is not normal to add it to everything willy-nilly > for no reason. I don't know why you want to make the topic as broad as all likely()/unlikely(), we are discussing likely()/unlikely() with pskb_may_pull() together, and particularly in bluetooth. Thanks.
