Re: [dpdk-dev] [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Mar 21, 2016 at 05:47:44PM +0000, Xie, Huawei wrote:
> On 3/18/2016 10:17 PM, Bruce Richardson wrote:
> > On Fri, Mar 18, 2016 at 01:47:29PM +0100, Mauricio Vásquez wrote:
> >> Hi,
> >>
> >>
> >> On Fri, Mar 18, 2016 at 11:35 AM, Thomas Monjalon <thomas.monjalon@6wind.com
> >>> wrote:
> >>> 2016-03-18 11:27, Olivier Matz:
> >>>> On 03/18/2016 11:18 AM, Bruce Richardson wrote:
> >>>>>>> +       /* Avoid the unnecessary cmpset operation below, which is
> >>> also
> >>>>>>> +        * potentially harmful when n equals 0. */
> >>>>>>> +       if (n == 0)
> >>>>>>>
> >>>>>> What about using unlikely here?
> >>>>>>
> >>>>> Unless there is a measurable performance increase by adding in
> >>> likely/unlikely
> >>>>> I'd suggest avoiding it's use. In general, likely/unlikely should only
> >>> be used
> >>>>> for things like catestrophic errors because the penalty for taking the
> >>> unlikely
> >>>>> leg of the code can be quite severe. For normal stuff, where the code
> >>> nearly
> >>>>> always goes one way in the branch but occasionally goes the other, the
> >>> hardware
> >>>>> branch predictors generally do a good enough job.
> >>>> Do you mean using likely/unlikely could be worst than not using it
> >>>> in this case?
> >>>>
> >>>> To me, using unlikely here is not a bad idea: it shows to the compiler
> >>>> and to the reader of the code that is case is not the usual case.
> >>> It would be nice to have a guideline section about likely/unlikely in
> >>> doc/guides/contributing/design.rst
> >>>
> >>> Bruce gave a talk at Dublin about this kind of things.
> >>> I'm sure he could contribute more design guidelines ;)
> >>>
> >> There is a small explanation in the section "Branch Prediction" of
> >> doc/guides/contributing/coding_style.rst, but I do not know if that is
> >> enough to understand when to use them.
> >>
> >> I've made a fast check and there are many PMDs that use them to check if
> >> number of packets is zero in the transmission function.
> > Yeah, and I wonder how many of those are actually necessary too :-)
> >
> > It's not a big deal either way, I just think the patch is fine as-is without
> > the extra macros.
> 
> IMO we use likely/unlikely in two cases, catastrophic errors and the
> code nearly always goes one way, i.e, preferred/favored fast path.
> Likely/unlikely helps not only for branch predication but also for cache

For branch prediction, anything after the first time through the code path
the prediction will be based on what happened before rather than any static
hints in the code.

> usage. The code generated for the likely path will directly follow the
> branch instruction. To me, it is reasonable enough to add unlikely for n
> == 0, which we don't expect to happen.
> I remember with/without likely, compiler could generate three kind of
> instructions. Didn't deep dive into it.
> 
> >
> > /Bruce
> >
>