| Message ID | 1277502400-9915-6-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Kevin Hilman |
| Headers | show |
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 of how to arrange code. And they only make the code less readable by adding extra braces and making ifs longer. Thus, NACK for this from my side.
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. > of how to arrange code. And they only make the code less readable by > adding extra braces and making ifs longer. > > Thus, NACK for this from my side. >
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.
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.
diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c index 7d13704..177299a 100644 --- a/arch/arm/mach-omap2/sr_device.c +++ b/arch/arm/mach-omap2/sr_device.c @@ -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)) { pr_warning("%s: Bad parameters! dev_data = %x," "dev_data->volts_supported = %x," "dev_data->volt_data = %x," @@ -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)) { pr_warning("%s: Bad parameters! dev_data = %x," "dev_data->volts_supported = %x," "dev_data->volt_data = %x," @@ -123,7 +124,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user) static int i; sr_data = kzalloc(sizeof(struct omap_sr_data), GFP_KERNEL); - if (!sr_data) { + if (unlikely(!sr_data)) { pr_err("%s: Unable to allocate memory for %s sr_data.Error!\n", __func__, oh->name); return -ENOMEM; @@ -150,7 +151,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user) sr_data->device_idle = omap_device_idle; sr_dev_data->volts_supported = omap_get_voltage_table(i, &sr_dev_data->volt_data); - if (!sr_dev_data->volts_supported) { + if (unlikely(!sr_dev_data->volts_supported)) { pr_warning("%s: No Voltage table registerd fo VDD%d. " "Something is really wrong\n", __func__, i + 1); @@ -162,7 +163,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user) od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data), omap_sr_latency, ARRAY_SIZE(omap_sr_latency), 0); - if (IS_ERR(od)) { + if (unlikely(IS_ERR(od))) { pr_warning("%s: Could not build omap_device for %s: %s.\n\n", __func__, name, oh->name); kfree(sr_data);