Messages in this thread | | | Date | Tue, 17 Mar 2020 07:56:07 +0000 | From | Morten Rasmussen <> | Subject | Re: [PATCH V2] sched: fair: Use the earliest break even |
| |
Hi Daniel,
First, I think letting the scheduler know about desired minimum idle times is an interesting optimization if the overhead can be kept at a minimum. I do have a few comments about the patch though.
On Thu, Mar 12, 2020 at 11:04:19AM +0100, Daniel Lezcano wrote: > On 12/03/2020 09:36, Vincent Guittot wrote: > > Hi Daniel, > > > > On Wed, 11 Mar 2020 at 21:28, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> In the idle CPU selection process occuring in the slow path via the > >> find_idlest_group_cpu() function, we pick up in priority an idle CPU > >> with the shallowest idle state otherwise we fall back to the least > >> loaded CPU. > > > > The idea makes sense but this path is only used by fork and exec so > > I'm not sure about the real impact > > I agree the fork / exec path is called much less often than the wake > path but it makes more sense for the decision.
Looking at the flow in find_idlest_cpu(), AFAICT, find_idlest_group_cpu() is not actually making the final choice of CPU, so going through a lot of trouble there looking at idle states is pointless. Is there something I don't see?
We fellow sd->child until groups == CPUs which which means that find_idlest_group() actually makes the final choice as the final group passed to find_idlest_group_cpu() is single-CPU group. The flow has been like that for years. Even before you added the initial idle-state awareness.
I agree with Vincent, if this should really make a difference it should include wake-ups existing tasks too. Although I'm aware it would be a more invasive change. As said from the beginning, the idea is fine, but the current implementation should not make any measurable difference?
> > >> In order to be more energy efficient but without impacting the > >> performances, let's use another criteria: the break even deadline. > >> > >> At idle time, when we store the idle state the CPU is entering in, we > >> compute the next deadline where the CPU could be woken up without > >> spending more energy to sleep.
I don't follow the argument that sleeping longer should improve energy consumption. The patch doesn't affect the number of idle state enter/exit cycles, so you spend the amount of energy on those transitions. The main change is that idle time get spread out, so CPUs are less likely to be in the process of entering an idle state when they are asked to wake back up again.
Isn't it fair to say that we expect the total number of wake-ups remains unchanged? Total busy and idle times across all CPUs should remain the same too? Unless chosen idle-state is changed, which I don't think we expect either, there should be no net effect on energy? The main benefit is reduced wake-up latency I think.
Regarding chosen idle state, I'm wondering how this patch affects the cpuidle governor's idle state selection. Could the spreading of wake-ups trick governor to pick a shallower idle-state for some idle CPUs because we actively spread wake-ups rather than consolidating them? Just a thought.
> >> > >> At the selection process, we use the shallowest CPU but in addition we > >> choose the one with the minimal break even deadline instead of relying > >> on the idle_timestamp. When the CPU is idle, the timestamp has less > >> meaning because the CPU could have wake up and sleep again several times > >> without exiting the idle loop. In this case the break even deadline is > >> more relevant as it increases the probability of choosing a CPU which > >> reached its break even.
I guess you could improve the idle time stamping without adding the break-even time, they don't have to go together?
> >> > >> Tested on: > >> - a synquacer 24 cores, 6 sched domains > >> - a hikey960 HMP 8 cores, 2 sched domains, with the EAS and energy probe > >> > >> sched/perf and messaging does not show a performance regression. Ran > >> 50 times schbench, adrestia and forkbench. > >> > >> The tools described at https://lwn.net/Articles/724935/ > >> > >> -------------------------------------------------------------- > >> | Synquacer | With break even | Without break even | > >> -------------------------------------------------------------- > >> | schbench *99.0th | 14844.8 | 15017.6 | > >> | adrestia / periodic | 57.95 | 57 | > >> | adrestia / single | 49.3 | 55.4 | > >> -------------------------------------------------------------- > > > > Have you got some figures or cpuidle statistics for the syncquacer ? > > No, and we just noticed the syncquacer has a bug in the firmware and > does not actually go to the idle states.
I would also like some statistics to help understanding what actually changes.
I did some measurements on TX2, which only has one idle-state. I don't see the same trends as you do. adrestia single seems to be most affected by the patch, but _increases_ with the break_even patch rather than decrease. I don't trust adrestia too much though as the time resolution is low on TX2.
TX2 tip break_even ---------------------------------------------------- adrestia / single 5.21 5.51 adrestia / periodic 5.75 5.67 schbench 99.0th 45465.6 45376.0 hackbench 27.9851 27.9775
Notes: adrestia: Avg of 100 runs: adrestia -l 25000 schbench: Avg of 10 runs: schbench -m16 -t64 hackbench: Avg of 10 runs: hackbench -g 20 -T 256 -l 100000
Morten
| |