Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/fair: Simplify some logic in update_sd_pick_busiest() | Date | Fri, 02 Feb 2024 18:40:21 +0100 |
| |
On 02/02/24 11:07, David Vernet wrote: > On Fri, Feb 02, 2024 at 06:01:22PM +0100, Valentin Schneider wrote: >> On 02/02/24 01:02, David Vernet 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(). >> > >> >> Given that's a different scope than what the rest of the patch touches, I'd >> rather see this as a separate patch. >> >> Other than that: >> Reviewed-by: Valentin Schneider <vschneid@redhat.com> > > Thanks, would you like me to send a follow-on series split into two with > your tag on both? Or were you just letting me know for next time? >
Well, I'm not picking up any patches, just reviewing them :) So yes I'd say re-send with the split and feel free to apply the tag on both.
> We could also update this check to only do a strict greater than to > avoid unnecessary writes, but I figured it was preferable to have no > logical changes for this iteration: > > return sgs->group_misfit_task_load >= busiest->group_misfit_task_load;
That's a good point, I don't think there was a specific reason for going with a lower-than rather than a lower-or-equal back then: cad68e552e77 ("sched/fair: Consider misfit tasks when load-balancing")
| |