[PATCH] driver core: Add branch prediction hints in really_probe()

[PATCH] driver core: Add branch prediction hints in really_probe()

[Muchun Song]

If condition is false in most cases. So, add an unlikely() to the if
condition, so that the optimizer assumes that the condition is false.

Signed-off-by: Muchun Song <smuchun@gmail.com>
---
 drivers/base/dd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 169412ee4ae8..8eba453c4e5d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -450,7 +450,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
 			   !drv->suppress_bind_attrs;
 
-	if (defer_all_probes) {
+	if (unlikely(defer_all_probes)) {
 		/*
 		 * Value of defer_all_probes can be set only by
 		 * device_defer_all_probes_enable() which, in turn, will call
@@ -508,7 +508,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 			goto probe_failed;
 	}
 
-	if (test_remove) {
+	if (unlikely(test_remove)) {
 		test_remove = false;
 
 		if (dev->bus->remove)
-- 
2.17.1

Re: [PATCH] driver core: Add branch prediction hints in really_probe()

[Rafael J. Wysocki]

On Tue, Nov 6, 2018 at 2:47 PM Muchun Song <smuchun@gmail.com> wrote:
>
> If condition is false in most cases. So, add an unlikely() to the if
> condition, so that the optimizer assumes that the condition is false.
>
> Signed-off-by: Muchun Song <smuchun@gmail.com>

Have you measured the practical impact of this patch in any way?

> ---
>  drivers/base/dd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 169412ee4ae8..8eba453c4e5d 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -450,7 +450,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>         bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>                            !drv->suppress_bind_attrs;
>
> -       if (defer_all_probes) {
> +       if (unlikely(defer_all_probes)) {
>                 /*
>                  * Value of defer_all_probes can be set only by
>                  * device_defer_all_probes_enable() which, in turn, will call
> @@ -508,7 +508,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>                         goto probe_failed;
>         }
>
> -       if (test_remove) {
> +       if (unlikely(test_remove)) {
>                 test_remove = false;
>
>                 if (dev->bus->remove)
> --
> 2.17.1
>

Re: [PATCH] driver core: Add branch prediction hints in really_probe()

[Muchun Song]

Hi Rafael,

If we want the driver core to test driver remove functions, we can
enable CONFIG_DEBUG_TEST_DRIVER_REMOVE. This option is
just for testing it. So, in most cases, the option is disabled and the if
condition is false. So I think we can add an unlikely() to it.

Rafael J. Wysocki <rafael@kernel.org> 于2018年11月6日周二 下午10:30写道：
>
> On Tue, Nov 6, 2018 at 2:47 PM Muchun Song <smuchun@gmail.com> wrote:
> >
> > If condition is false in most cases. So, add an unlikely() to the if
> > condition, so that the optimizer assumes that the condition is false.
> >
> > Signed-off-by: Muchun Song <smuchun@gmail.com>
>
> Have you measured the practical impact of this patch in any way?
>
> > ---
> >  drivers/base/dd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 169412ee4ae8..8eba453c4e5d 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -450,7 +450,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >         bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
> >                            !drv->suppress_bind_attrs;
> >
> > -       if (defer_all_probes) {
> > +       if (unlikely(defer_all_probes)) {
> >                 /*
> >                  * Value of defer_all_probes can be set only by
> >                  * device_defer_all_probes_enable() which, in turn, will call
> > @@ -508,7 +508,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >                         goto probe_failed;
> >         }
> >
> > -       if (test_remove) {
> > +       if (unlikely(test_remove)) {
> >                 test_remove = false;
> >
> >                 if (dev->bus->remove)
> > --
> > 2.17.1
> >

Re: [PATCH] driver core: Add branch prediction hints in really_probe()

[Rafael J. Wysocki]

On Tue, Nov 6, 2018 at 3:43 PM Muchun Song <smuchun@gmail.com> wrote:
>
> Hi Rafael,
>
> If we want the driver core to test driver remove functions, we can
> enable CONFIG_DEBUG_TEST_DRIVER_REMOVE. This option is
> just for testing it. So, in most cases, the option is disabled and the if
> condition is false. So I think we can add an unlikely() to it.

Yes, it can be added there, but does it really need to be added?

If the conditions are false all the time, the branch predictor in the
processor should be able to deal with it just fine.

And if they are false already at build time, the compiler should just
optimize them away.

Re: [PATCH] driver core: Add branch prediction hints in really_probe()

[Muchun Song]

Hi Rafael,

>
> On Tue, Nov 6, 2018 at 3:43 PM Muchun Song <smuchun@gmail.com> wrote:
> >
> > Hi Rafael,
> >
> > If we want the driver core to test driver remove functions, we can
> > enable CONFIG_DEBUG_TEST_DRIVER_REMOVE. This option is
> > just for testing it. So, in most cases, the option is disabled and the if
> > condition is false. So I think we can add an unlikely() to it.
>
> Yes, it can be added there, but does it really need to be added?
>
> If the conditions are false all the time, the branch predictor in the
> processor should be able to deal with it just fine.
>
> And if they are false already at build time, the compiler should just
> optimize them away.

Thank you for your explanation. I really didn't take into account the
situation you said. Yeah, the compiler can optimize them away or
the processor can deal with it.

So,  we don't need to add unlikely() to it. The patch does't make sense.
Thanks.

Re: [PATCH] driver core: Add branch prediction hints in really_probe()

[Greg KH]

On Tue, Nov 06, 2018 at 09:46:30PM +0800, Muchun Song wrote:
> If condition is false in most cases. So, add an unlikely() to the if
> condition, so that the optimizer assumes that the condition is false.
> 
> Signed-off-by: Muchun Song <smuchun@gmail.com>

As Rafael said, don't do stuff like this unless you can actually measure
the difference.  Last time we tested the kernel for this a few years
ago, about 75% of the unlikely/likely additions were incorrect, removing
them made the kernel faster.  Turns out the compiler and cpu know better
than developers do :)

Also, the probe() path is never a hot-path to worry about stuff like
this.

thanks,

greg k-h