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

[Artem Bityutskiy <dedekind1@gmail.com>]

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.


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html