Re: [PATCH v2 3/3] sched/fair: schedutil: explicit update only when required

From: Vincent Guittot
Date: Tue May 15 2018 - 06:12:50 EST

Hi Patrick,

On 11 May 2018 at 15:15, Patrick Bellasi <patrick.bellasi@xxxxxxx> wrote:
> Schedutil updates for FAIR tasks are triggered implicitly each time a
> cfs_rq's utilization is updated via cfs_rq_util_change(), currently
> called by update_cfs_rq_load_avg(), when the utilization of a cfs_rq has
> changed, and {attach,detach}_entity_load_avg().
> This design is based on the idea that "we should callback schedutil
> frequently enough" to properly update the CPU frequency at every
> utilization change. However, such an integration strategy has also
> some downsides:
> - schedutil updates are triggered by RQ's load updates, which makes
> sense in general but it does not allow to know exactly which other RQ
> related information have been updated.
> Recently, for example, we had issues due to schedutil dependencies on
> cfs_rq->h_nr_running and estimated utilization updates.
> - cfs_rq_util_change() is mainly a wrapper function for an already
> existing "public API", cpufreq_update_util(), which is required
> just to ensure we actually update schedutil only when we are updating
> a root cfs_rq.
> Thus, especially when task groups are in use, most of the calls to
> this wrapper function are not required.
> - the usage of a wrapper function is not completely consistent across
> fair.c, since we could still need additional explicit calls to
> cpufreq_update_util().
> For example this already happens to report the IOWAIT boot flag in
> the wakeup path.
> - it makes it hard to integrate new features since it could require to
> change other function prototypes just to pass in an additional flag,
> as it happened for example in commit:
> commit ea14b57e8a18 ("sched/cpufreq: Provide migration hint")
> All the above considered, let's make schedutil updates more explicit in
> fair.c by removing the cfs_rq_util_change() wrapper function in favour
> of the existing cpufreq_update_util() public API.

removing the wrapper makes sense

> This can be done by calling cpufreq_update_util() explicitly in the few
> call sites where it really makes sense and when all the (potentially)
> required cfs_rq's information have been updated.
> This patch mainly removes code and adds explicit schedutil updates
> only when we:
> - {enqueue,dequeue}_task_fair() a task to/from the root cfs_rq

The task's utilization is added during attach/detach and not during
the enqueue/dequeue.

> - (un)throttle_cfs_rq() a set of tasks up to the root cfs_rq
> - task_tick_fair() to update the utilization of the root cfs_rq
> All the other code paths, currently _indirectly_ covered by a call to
> update_load_avg(), are still covered. Indeed, some paths already imply
> enqueue/dequeue calls:
> - switch_{to,from}_fair()
> - sched_move_task()
> while others are followed by enqueue/dequeue calls:
> - cpu_cgroup_fork() and
> post_init_entity_util_avg():
> are used at wakeup_new_task() time and thus already followed by an
> enqueue_task_fair()
> - migrate_task_rq_fair():
> updates the removed utilization but not the actual cfs_rq
> utilization, which is updated by a following sched event

The removed utilization will be applied during next call to
update_cfs_rq_load_avg() which means at next call to update_load_avg()
or update_blocked_averages()

I'm going to continue on the longer reply that you made to Joel

> This new proposal allows also to better aggregate schedutil related
> flags, which are required only at enqueue_task_fair() time.
> IOWAIT and MIGRATION flags are now requested only when a task is
> actually visible at the root cfs_rq level.
> Signed-off-by: Patrick Bellasi <patrick.bellasi@xxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Cc: Joel Fernandes <joelaf@xxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-pm@xxxxxxxxxxxxxxx
> ---