Re: [PATCH] eal/common: better likely() and unlikely()

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 14 Jan 2018 01:45:42 +0300
Aleksey Baulin <Aleksey.Baulin@gmail.com> wrote:

> Please see my comments inline.
> 
> On Sun, Jan 14, 2018 at 1:24 AM, Thomas Monjalon <thomas@monjalon.net>
> wrote:
> 
> > Hi,
> >
> > I moved your top-post below and did some comments inline.
> > More opinions are welcome.
> >
> > 13/01/2018 23:05, Aleksey Baulin:  
> > > On Fri, Jan 12, 2018 at 6:35 PM, Thomas Monjalon <thomas@monjalon.net>
> > > wrote:  
> > > > 21/11/2017 08:05, Aleksey Baulin:  
> > > > > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com  
> > >  
> > > > wrote:  
> > > > > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <  
> > > > aleksey.baulin@gmail.com>  
> > > > > > wrote:  
> > > > > > > -#define unlikely(x)  __builtin_expect((x),0)
> > > > > > > +#define unlikely(x)  __builtin_expect(!!(x), 0)  
> > > > > >
> > > > > > I have not looked at the generated code, but does this add some  
> > extra  
> > > > > > instruction now to do the !!(x) ?  
> > > > >
> > > > > Sorry for late response. Jim had given the correct answer already.
> > > > > You won't get an extra instruction with compiler optimization turned  
> > on.  
> > > >
> > > > So this patch is adding an instruction in not optimized binary.
> > > > I don't understand the benefit.
> > > > Is it just to avoid to make pointer comparison explicit?
> > > > likely(pointer != NULL) looks better than likely(pointer).  
> > >
> > > This is an interesting question. Perhaps, even a philosophical one. :-)
> > >
> > > 'likely(pointer)' is a perfectly legal statement in C language, as well  
> > as  
> > > a concise one as
> > > compared to a more explicit (and redundant/superfluous) 'likely(pointer  
> > !=  
> > > NULL)'. If you
> > > _require_ this kind of explicitness in cases like this in the code style,
> > > then I have no
> > > argument here. However, I don't see that anywhere.  
> >
> > It is stated here:
> >         http://dpdk.org/doc/guides/contributing/coding_style.
> > html#null-pointers  
> 
> 
> ​Oh, thanks for pointing that out! I am sincerely ashamed for missing it.​
> I lose that argument as I certainly do submit to the coding style. My only
> excuse is that I am actually developing an app and not the DPDK core.
> 
> 
> > > There're other cases of explicitness, with the most widespread being a
> > > series of logical and
> > > compare operations in one statement. For instance, 'if (a > b && a < c)'.
> > > Explicitness would
> > > require writing it like this: 'if ((a > b) && (a < c))'. I've seen cases  
> > on  
> > > this list where that was
> > > frowned upon as it's totally unnecessary due to C operator precedence
> > > rules, even though
> > > those statements, I think, looked better to their authors (actually, they
> > > do to me). Granted,
> > > it didn't lead to compiler errors, which is the case with the current
> > > implementation of 'likely()'.
> > >
> > > So, my answer to the question is yes, it's to avoid making pointer
> > > comparison explicit. I would
> > > add though, that it is to avoid making a perfectly legal C statement an
> > > illegal one, as with the
> > > way the current macro is constructed, compiler emits an error when DPDK  
> > is  
> > > built. I write in C
> > > for many years with the most time spent in kernels, Linux and not, and I
> > > find it unnatural to
> > > always add a redundant '!= NULL' just to satisfy the current macro
> > > implementation. I would
> > > have to accept that though if it's a requirement clearly stated somewhere
> > > like a code style.
> > >
> > > As for an extra instruction, I believe that everything in DPDK  
> > distribution  
> > > is compiled with
> > > optimization. So the execution speed in not a concern here. Perhaps there
> > > are cases where
> > > it's compiled without optimization, like debugging, but then I believe  
> > it's  
> > > a non-issue.  
> >
> > Yes you're right about optimization.
> > But can we be 100% sure that it is always well optimized?
> >  
> 
> ​I believe we can. I hope we get other opinions as well.​
> 
> > Hope my explanations shed some more light on this patch. :-)
> >
> > If we can be sure that there is no cost on optimized code,
> > I am not against this patch.
> > It may be especially useful when not complying to the DPDK
> > coding rules, like in applications.
> >  
> 
> ​Yes, that's exactly my case. Thanks.​
> 

My opinion is that the DPDK likely() macro must behave exactly the same
as the kernel and other projects. Doing something unique is not a great benefit.