From: Markus Elfring <[email protected]>
Date: Sat, 6 Jul 2019 16:00:13 +0200
Avoid an extra function call by using a ternary operator instead of
a conditional statement.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/sched/topology.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index f751ce0b783e..6190eb52c30a 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -886,11 +886,7 @@ build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
return NULL;
sg_span = sched_group_span(sg);
- if (sd->child)
- cpumask_copy(sg_span, sched_domain_span(sd->child));
- else
- cpumask_copy(sg_span, sched_domain_span(sd));
-
+ cpumask_copy(sg_span, sched_domain_span(sd->child ? sd->child : sd));
atomic_inc(&sg->ref);
return sg;
}
--
2.22.0
* Markus Elfring <[email protected]> [2019-07-06 16:05:17]:
> From: Markus Elfring <[email protected]>
> Date: Sat, 6 Jul 2019 16:00:13 +0200
>
> Avoid an extra function call by using a ternary operator instead of
> a conditional statement.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> kernel/sched/topology.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index f751ce0b783e..6190eb52c30a 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -886,11 +886,7 @@ build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
> return NULL;
>
> sg_span = sched_group_span(sg);
> - if (sd->child)
> - cpumask_copy(sg_span, sched_domain_span(sd->child));
> - else
> - cpumask_copy(sg_span, sched_domain_span(sd));
> -
> + cpumask_copy(sg_span, sched_domain_span(sd->child ? sd->child : sd));
At runtime, Are we avoiding a function call?
However I think we are avoiding a branch instead of a conditional, which may
be beneficial.
> atomic_inc(&sg->ref);
> return sg;
> }
> --
> 2.22.0
>
--
Thanks and Regards
Srikar Dronamraju
On 06.07.19 19:22, Srikar Dronamraju wrote:
> * Markus Elfring <[email protected]> [2019-07-06 16:05:17]:
>
>> From: Markus Elfring <[email protected]>
>> Date: Sat, 6 Jul 2019 16:00:13 +0200
>>
>> Avoid an extra function call by using a ternary operator instead of
>> a conditional statement.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <[email protected]>
>> ---
>> kernel/sched/topology.c | 6 +-----
>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index f751ce0b783e..6190eb52c30a 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -886,11 +886,7 @@ build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
>> return NULL;
>>
>> sg_span = sched_group_span(sg);
>> - if (sd->child)
>> - cpumask_copy(sg_span, sched_domain_span(sd->child));
>> - else
>> - cpumask_copy(sg_span, sched_domain_span(sd));
>> -
>> + cpumask_copy(sg_span, sched_domain_span(sd->child ? sd->child : sd));
>
> At runtime, Are we avoiding a function call?
> However I think we are avoiding a branch instead of a conditional, which may
> be beneficial.
If you're assuming the compiler doesn't already optimize that (no idea
whether gcc really does that).
@Markus: could you check what gcc is actually generating out of both the
old and your new version ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
On Sat, Jul 06, 2019 at 10:52:23PM +0530, Srikar Dronamraju wrote:
> * Markus Elfring <[email protected]> [2019-07-06 16:05:17]:
>
> > From: Markus Elfring <[email protected]>
> > Date: Sat, 6 Jul 2019 16:00:13 +0200
> >
> > Avoid an extra function call by using a ternary operator instead of
> > a conditional statement.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <[email protected]>
> > ---
> > kernel/sched/topology.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index f751ce0b783e..6190eb52c30a 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -886,11 +886,7 @@ build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
> > return NULL;
> >
> > sg_span = sched_group_span(sg);
> > - if (sd->child)
> > - cpumask_copy(sg_span, sched_domain_span(sd->child));
> > - else
> > - cpumask_copy(sg_span, sched_domain_span(sd));
> > -
> > + cpumask_copy(sg_span, sched_domain_span(sd->child ? sd->child : sd));
>
> At runtime, Are we avoiding a function call?
> However I think we are avoiding a branch instead of a conditional, which may
> be beneficial.
It all depends on what the compiler does; also this is super slow path
stuff and the patch makes code less readable (IMO).
* Peter Zijlstra <[email protected]> [2019-07-08 12:23:12]:
> On Sat, Jul 06, 2019 at 10:52:23PM +0530, Srikar Dronamraju wrote:
> > * Markus Elfring <[email protected]> [2019-07-06 16:05:17]:
> >
> > > From: Markus Elfring <[email protected]>
> > > Date: Sat, 6 Jul 2019 16:00:13 +0200
> > >
> > > Avoid an extra function call by using a ternary operator instead of
> > > a conditional statement.
> > >
> > > This issue was detected by using the Coccinelle software.
> > >
> > > Signed-off-by: Markus Elfring <[email protected]>
> > > ---
> > > kernel/sched/topology.c | 6 +-----
> > > 1 file changed, 1 insertion(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > > index f751ce0b783e..6190eb52c30a 100644
> > > --- a/kernel/sched/topology.c
> > > +++ b/kernel/sched/topology.c
> > > @@ -886,11 +886,7 @@ build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
> > > return NULL;
> > >
> > > sg_span = sched_group_span(sg);
> > > - if (sd->child)
> > > - cpumask_copy(sg_span, sched_domain_span(sd->child));
> > > - else
> > > - cpumask_copy(sg_span, sched_domain_span(sd));
> > > -
> > > + cpumask_copy(sg_span, sched_domain_span(sd->child ? sd->child : sd));
> >
> > At runtime, Are we avoiding a function call?
> > However I think we are avoiding a branch instead of a conditional, which may
> > be beneficial.
>
> It all depends on what the compiler does; also this is super slow path
> stuff and the patch makes code less readable (IMO).
>
Yes, it definitely makes code readable. I was only commenting on the
changelog/subject which says avoids a function call which I think it
doesn't.
--
Thanks and Regards
Srikar Dronamraju
* Enrico Weigelt, metux IT consult <[email protected]> [2019-07-08 11:38:58]:
> >
> > At runtime, Are we avoiding a function call?
> > However I think we are avoiding a branch instead of a conditional, which may
> > be beneficial.
>
> If you're assuming the compiler doesn't already optimize that (no idea
> whether gcc really does that).
>
> @Markus: could you check what gcc is actually generating out of both the
> old and your new version ?
>
>
I had already tried looking at the object files both on X86 and PowerPc and
in both cases (with and without patch) the generated code differs.
> --mtx
>
> --
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering
> [email protected] -- +49-151-27565287
>
--
Thanks and Regards
Srikar Dronamraju
> Yes, it definitely makes code readable.
Some developers seem to get difficulties with this view.
> I was only commenting on the changelog/subject which says
> avoids a function call which I think it doesn't.
Do you prefer an other wording for calling functions only once
with a selected parameter value instead of calling them (unoptimised)
within two branches of an if statement?
Regards,
Markus