update_sd_pick_busiest() (and its caller) has some room for small
optimizations, and some improvements in readability.
- In update_sd_lb_stats(), we're using a goto to skip a single if check.
Let's remove the goto and just add another condition to the if.
- In update_sd_pick_busiest(), only update a group_misfit_task group to
be the busiest if it has strictly more load than the current busiest
task, rather than >= the load.
- When comparing the current struct sched_group with the yet-busiest
domain in update_sd_pick_busiest(), if the two groups have the same
group type, we're currently doing a bit of unnecessary work for any
group >= group_misfit_task. We're comparing the two groups, and then
returning only if false (the group in question is not the busiest).
Othewise, we break, do an extra unnecessary conditional check that's
vacuously false for any group type > group_fully_busy, and then always
return true. This patch series has us instead simply return directly
in the switch statement, saving some bytes in load_balance().
---
v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2 changes:
- Split the patch series into separate patches (Valentin)
- Update the group_misfit_task busiest check to use strict inequality
David Vernet (3):
sched/fair: Remove unnecessary goto in update_sd_lb_stats()
sched/fair: Do strict inequality check for busiest misfit task group
sched/fair: Simplify some logic in update_sd_pick_busiest()
kernel/sched/fair.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
--
2.43.0
In update_sd_lb_stats(), when we're iterating over the sched groups that
comprise a sched domain, we're skipping the call to
update_sd_pick_busiest() for the sched group that contains the local /
destination CPU. We use a goto to skip the call, but we could just as
easily check !local_group, as there's no other logic that we need to
skip with the goto. Let's remove the goto, and check for !local_group in
the if statement instead.
Reviewed-by: Valentin Schneider <[email protected]>
Signed-off-by: David Vernet <[email protected]>
---
kernel/sched/fair.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b803030c3a03..e7519ea434b1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10578,16 +10578,11 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
update_sg_lb_stats(env, sds, sg, sgs, &sg_status);
- if (local_group)
- goto next_group;
-
-
- if (update_sd_pick_busiest(env, sds, sg, sgs)) {
+ if (!local_group && update_sd_pick_busiest(env, sds, sg, sgs)) {
sds->busiest = sg;
sds->busiest_stat = *sgs;
}
-next_group:
/* Now, start updating sd_lb_stats */
sds->total_load += sgs->group_load;
sds->total_capacity += sgs->group_capacity;
--
2.43.0
On Sun, 4 Feb 2024 at 05:46, David Vernet <[email protected]> wrote:
>
> In update_sd_lb_stats(), when we're iterating over the sched groups that
> comprise a sched domain, we're skipping the call to
> update_sd_pick_busiest() for the sched group that contains the local /
> destination CPU. We use a goto to skip the call, but we could just as
> easily check !local_group, as there's no other logic that we need to
> skip with the goto. Let's remove the goto, and check for !local_group in
> the if statement instead.
>
> Reviewed-by: Valentin Schneider <[email protected]>
> Signed-off-by: David Vernet <[email protected]>
Reviewed-by: Vincent Guittot <[email protected]>
> ---
> kernel/sched/fair.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b803030c3a03..e7519ea434b1 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10578,16 +10578,11 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>
> update_sg_lb_stats(env, sds, sg, sgs, &sg_status);
>
> - if (local_group)
> - goto next_group;
> -
> -
> - if (update_sd_pick_busiest(env, sds, sg, sgs)) {
> + if (!local_group && update_sd_pick_busiest(env, sds, sg, sgs)) {
> sds->busiest = sg;
> sds->busiest_stat = *sgs;
> }
>
> -next_group:
> /* Now, start updating sd_lb_stats */
> sds->total_load += sgs->group_load;
> sds->total_capacity += sgs->group_capacity;
> --
> 2.43.0
>