Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Nishanth Menon <nm@ti.com>]

Artem Bityutskiy had written, on 07/09/2010 09:15 AM, the following:
> On Fri, 2010-07-09 at 08:40 -0500, Nishanth Menon wrote:
>> Artem Bityutskiy had written, on 07/09/2010 08:12 AM, the following:
>>> On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
>>>> Add unlikely checks to better optimize the rare occurrance of
>>>> erroneous conditions.
>>>>
>>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Cc: Thara Gopinath <thara@ti.com>
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> unlikely and friends make sens only in realy hot path places. In other
>>> places like you touch, they are pointless - better let gcc make a choice
>>> @@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
>>>  {
>>>  	int i;
>>>  
>>> -	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
>>> -			!dev_data->efuse_nvalues_offs) {
>>> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
>>> +			!dev_data->volt_data ||
>>> +			!dev_data->efuse_nvalues_offs)) {
>>> @@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
>>>  {
>>>  	int i;
>>>  
>>> -	if (!dev_data || !dev_data->volts_supported ||
>>> -			!dev_data->volt_data || !dev_data->test_nvalues) {
>>> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
>>> +			!dev_data->volt_data || !dev_data->test_nvalues)) {
>> and other places, why do you think that these are checks that should be 
>> expected? - would be great if you can explain inline to the patch which 
>> unlikely checks dont make sense.
>>
>> static functions such as these are helpers for the maincode, unless 
>> something horribly wrong occurs within the codepath calling these 
>> helpers, they are not expected to be invalid parameters. hence the 
>> rationale for adding unlikely.
> 
> Sorry, I do not really understand you. All I said is that
> unlikely()/likely() are usually used in hot-paths / tight loops.
> 
> unlikely()/likely() are micro optimization. They make no difference when
> you use them in initialization paths.
> 
> So what I said, that in your patch they will make no difference
> performance wise. So no benefits.
> 
> But they make if () statements a bit more difficult to read, this is a
> drawback.
> 
> So your patch introduces no benefits, just a drawback. Thus, it is not
> good.
> 
> There were many flamewars about unlikely and likely in lkml in the past.
> And the outcome was always - do not use them anywhere except of
> performance-critical tight loops / hot paths.
> 
> 
alright.. thanks for the review. will drop this patch.

-- 
Regards,
Nishanth Menon