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

["Xie, Huawei" <huawei.xie@intel.com>]

On 3/22/2016 6:13 PM, Richardson, Bruce wrote:
> 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.

Yes, maybe i didn't make myself clear? My main concern isn't about
branch predication...

>> 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
>>>