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@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Thara Gopinath <thara@xxxxxx>
> >>
> >> Signed-off-by: Nishanth Menon <nm@xxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
