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.
Let's just return directly in the switch statement instead. This doesn't
change the size of vmlinux with llvm 17 (not surprising given that all
of this is inlined in load_balance()), but it does shrink load_balance()
by 88 bytes on x86. Given that it also improves readability, this seems
worth doing.
As a bonus, remove an unnecessary goto in update_sd_lb_stats().
Reviewed-by: Valentin Schneider <[email protected]>
Signed-off-by: David Vernet <[email protected]>
---
kernel/sched/fair.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 76d03106040d..fa049f866461 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10006,9 +10006,7 @@ static bool update_sd_pick_busiest(struct lb_env *env,
switch (sgs->group_type) {
case group_overloaded:
/* Select the overloaded group with highest avg_load. */
- if (sgs->avg_load <= busiest->avg_load)
- return false;
- break;
+ return sgs->avg_load > busiest->avg_load;
case group_imbalanced:
/*
@@ -10019,18 +10017,14 @@ static bool update_sd_pick_busiest(struct lb_env *env,
case group_asym_packing:
/* Prefer to move from lowest priority CPU's work */
- if (sched_asym_prefer(sg->asym_prefer_cpu, sds->busiest->asym_prefer_cpu))
- return false;
- break;
+ return sched_asym_prefer(sds->busiest->asym_prefer_cpu, sg->asym_prefer_cpu);
case group_misfit_task:
/*
* If we have more than one misfit sg go with the biggest
* misfit.
*/
- if (sgs->group_misfit_task_load <= busiest->group_misfit_task_load)
- return false;
- break;
+ return sgs->group_misfit_task_load > busiest->group_misfit_task_load;
case group_smt_balance:
/*
--
2.43.0
On Sun, 4 Feb 2024 at 05:46, David Vernet <[email protected]> wrote:
>
> 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.
>
> Let's just return directly in the switch statement instead. This doesn't
> change the size of vmlinux with llvm 17 (not surprising given that all
> of this is inlined in load_balance()), but it does shrink load_balance()
> by 88 bytes on x86. Given that it also improves readability, this seems
> worth doing.
>
> As a bonus, remove an unnecessary goto in update_sd_lb_stats().
The line above is not relevant to the content of the patch.
Other than that
Reviewed-by: Vincent Guittot <[email protected]>
>
> Reviewed-by: Valentin Schneider <[email protected]>
> Signed-off-by: David Vernet <[email protected]>
> ---
> kernel/sched/fair.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 76d03106040d..fa049f866461 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10006,9 +10006,7 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> switch (sgs->group_type) {
> case group_overloaded:
> /* Select the overloaded group with highest avg_load. */
> - if (sgs->avg_load <= busiest->avg_load)
> - return false;
> - break;
> + return sgs->avg_load > busiest->avg_load;
>
> case group_imbalanced:
> /*
> @@ -10019,18 +10017,14 @@ static bool update_sd_pick_busiest(struct lb_env *env,
>
> case group_asym_packing:
> /* Prefer to move from lowest priority CPU's work */
> - if (sched_asym_prefer(sg->asym_prefer_cpu, sds->busiest->asym_prefer_cpu))
> - return false;
> - break;
> + return sched_asym_prefer(sds->busiest->asym_prefer_cpu, sg->asym_prefer_cpu);
>
> case group_misfit_task:
> /*
> * If we have more than one misfit sg go with the biggest
> * misfit.
> */
> - if (sgs->group_misfit_task_load <= busiest->group_misfit_task_load)
> - return false;
> - break;
> + return sgs->group_misfit_task_load > busiest->group_misfit_task_load;
>
> case group_smt_balance:
> /*
> --
> 2.43.0
>
On Sun, Feb 04, 2024 at 12:48:11PM +0100, Vincent Guittot wrote:
> On Sun, 4 Feb 2024 at 05:46, David Vernet <[email protected]> wrote:
> >
> > 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.
> >
> > Let's just return directly in the switch statement instead. This doesn't
> > change the size of vmlinux with llvm 17 (not surprising given that all
> > of this is inlined in load_balance()), but it does shrink load_balance()
> > by 88 bytes on x86. Given that it also improves readability, this seems
> > worth doing.
> >
> > As a bonus, remove an unnecessary goto in update_sd_lb_stats().
>
> The line above is not relevant to the content of the patch.
Ah, thanks for catching that.
Should I send a v3 of the patch set? Or should I just let whomever
applies remove that line?
> Other than that
>
> Reviewed-by: Vincent Guittot <[email protected]>
Thanks,
David