lkml.org 
[lkml]   [2020]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [RFC] sched: restrict iowait boost for boosted task only
On 01/23/20 16:28, Wei Wang wrote:
> Currently iowait doesn't distinguish background/foreground tasks.
> This may not be a problem in servers but could impact devices that
> are more sensitive on energy consumption. [1]
> In Android world, we have seen many cases where device run to high and
> inefficient frequency unnecessarily when running some background task
> which are interacting I/O.
>
> The ultimate goal is to only apply iowait boost on UX related tasks and
> leave the rest for EAS scheduler for better efficiency.
>
> This patch limits iowait boost for tasks which are associated with boost
> signal from user land. On Android specifically, those are foreground or
> top app tasks.
>
> [1] ANDROID discussion:
> https://android-review.googlesource.com/c/kernel/common/+/1215695
>
> Signed-off-by: Wei Wang <wvw@google.com>
> ---
> kernel/sched/fair.c | 4 +++-
> kernel/sched/sched.h | 12 ++++++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ba749f579714..221c0fbcea0e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5220,8 +5220,10 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> * If in_iowait is set, the code below may not trigger any cpufreq
> * utilization updates, so do it here explicitly with the IOWAIT flag
> * passed.
> + * If CONFIG_ENERGY_MODEL and CONFIG_UCLAMP_TASK are both configured,
> + * only boost for tasks set with non-null min-clamp.
> */
> - if (p->in_iowait)
> + if (iowait_boosted(p))
> cpufreq_update_util(rq, SCHED_CPUFREQ_IOWAIT);
>
> for_each_sched_entity(se) {
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 280a3c735935..a13d49d835ed 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2341,6 +2341,18 @@ static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> }
> #endif /* CONFIG_UCLAMP_TASK */
>
> +#if defined(CONFIG_ENERGY_MODEL) && defined(CONFIG_UCLAMP_TASK)

Why depend on CONFIG_ENERGY_MODEL? Laptops are battery powered and might not
have this option enabled. Do we really need energy model here?

> +static inline bool iowait_boosted(struct task_struct *p)
> +{
> + return p->in_iowait && uclamp_eff_value(p, UCLAMP_MIN) > 0;

I think this is overloading the usage of util clamp. You're basically using
cpu.uclamp.min to temporarily switch iowait boost on/off.

Isn't it better to add a new cgroup attribute to toggle this feature?

The problem does seem generic enough and could benefit other battery-powered
devices outside of the Android world. I don't think the dependency on uclamp &&
energy model are necessary to solve this.

Thanks

--
Qais Yousef

> +}
> +#else /* defined(CONFIG_ENERGY_MODEL) && defined(CONFIG_UCLAMP_TASK) */
> +static inline bool iowait_boosted(struct task_struct *p)
> +{
> + return p->in_iowait;
> +}
> +#endif /* defined(CONFIG_ENERGY_MODEL) && defined(CONFIG_UCLAMP_TASK) */
> +
> #ifdef arch_scale_freq_capacity
> # ifndef arch_scale_freq_invariant
> # define arch_scale_freq_invariant() true
> --
> 2.25.0.341.g760bfbb309-goog
>

\
 
 \ /
  Last update: 2020-01-24 03:53    [W:0.947 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site