Messages in this thread | | | Date | Fri, 24 Jan 2020 02:52:39 +0000 | From | Qais Yousef <> | 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@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 >
| |