linux-kernel - Re: [PATCH] [RFC] sched: restrict iowait boost for boosted task only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200124025238.jsf36n6w4rrn2ehc@e107158-lin>
Date:   Fri, 24 Jan 2020 02:52:39 +0000
From:   Qais Yousef <qais.yousef@....com>
To:     Wei Wang <wvw@...gle.com>
Cc:     wei.vince.wang@...il.com, dietmar.eggemann@....com,
        valentin.schneider@....com, chris.redpath@....com,
        qperret@...gle.com, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [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@...gle.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
>