On Tuesday, March 5, 2019 9:32:02 AM CET Peter Zijlstra wrote:
> On Mon, Mar 04, 2019 at 07:11:01PM +0000, Quentin Perret wrote:
>
> > So yeah, that works for me.
>
> Chunyan, Vincent; can you verify the below cures your ill?
>
> ---
> Subject: sched/cpufreq: Fix 32bit math overflow
>
> Vincent Wang reported that get_next_freq() has a mult overflow issue on
> 32bit platforms in the IOWAIT boost case, since in that case {util,max}
> are in freq units instead of capacity units.
>
> Solve this by moving the IOWAIT boost to capacity units. And since this
> means @max is constant; simplify the code.
>
> Cc: Chunyan Zhang <[email protected]>
> Reported-by: Vincent Wang <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> kernel/sched/cpufreq_schedutil.c | 58
> +++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index 2efe629425be..72b62ac1c7c2 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -48,10 +48,10 @@ struct sugov_cpu {
>
> bool iowait_boost_pending;
> unsigned int iowait_boost;
> - unsigned int iowait_boost_max;
> u64 last_update;
>
> unsigned long bw_dl;
> + unsigned long min;
> unsigned long max;
>
> /* The field below is for single-CPU policies only: */
> @@ -303,8 +303,7 @@ static bool sugov_iowait_reset(struct sugov_cpu *sg_cpu,
> u64 time,
> if (delta_ns <= TICK_NSEC)
> return false;
>
> - sg_cpu->iowait_boost = set_iowait_boost
> - ? sg_cpu->sg_policy->policy->min : 0;
> + sg_cpu->iowait_boost = set_iowait_boost ? sg_cpu->min : 0;
> sg_cpu->iowait_boost_pending = set_iowait_boost;
>
> return true;
> @@ -344,14 +343,12 @@ static void sugov_iowait_boost(struct sugov_cpu
> *sg_cpu, u64 time,
>
> /* Double the boost at each request */
> if (sg_cpu->iowait_boost) {
> - sg_cpu->iowait_boost <<= 1;
> - if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> + sg_cpu->iowait_boost = min(sg_cpu->iowait_boost << 1,
> SCHED_CAPACITY_SCALE);
> return;
> }
>
> /* First wakeup after IO: start with minimum boost */
> - sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
> + sg_cpu->iowait_boost = sg_cpu->min;
> }
>
> /**
> @@ -373,47 +370,38 @@ static void sugov_iowait_boost(struct sugov_cpu
> *sg_cpu, u64 time,
> * This mechanism is designed to boost high frequently IO waiting tasks,
> while
> * being more conservative on tasks which does sporadic IO operations.
> */
> -static void sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
> - unsigned long *util, unsigned long *max)
> +static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
> + unsigned long util, unsigned long max)
> {
> - unsigned int boost_util, boost_max;
> + unsigned long boost;
>
> /* No boost currently required */
> if (!sg_cpu->iowait_boost)
> - return;
> + return util;
>
> /* Reset boost if the CPU appears to have been idle enough */
> if (sugov_iowait_reset(sg_cpu, time, false))
> - return;
> + return util;
>
> - /*
> - * An IO waiting task has just woken up:
> - * allow to further double the boost value
> - */
> - if (sg_cpu->iowait_boost_pending) {
> - sg_cpu->iowait_boost_pending = false;
> - } else {
> + if (!sg_cpu->iowait_boost_pending) {
> /*
> - * Otherwise: reduce the boost value and disable it when we
> - * reach the minimum.
> + * No boost pending; reduce the boost value.
> */
> sg_cpu->iowait_boost >>= 1;
> - if (sg_cpu->iowait_boost < sg_cpu->sg_policy->policy->min) {
> + if (sg_cpu->iowait_boost < sg_cpu->min) {
> sg_cpu->iowait_boost = 0;
> - return;
> + return util;
> }
> }
>
> + sg_cpu->iowait_boost_pending = false;
> +
> /*
> - * Apply the current boost value: a CPU is boosted only if its current
> - * utilization is smaller then the current IO boost level.
> + * @util is already in capacity scale; convert iowait_boost
> + * into the same scale so we can compare.
> */
> - boost_util = sg_cpu->iowait_boost;
> - boost_max = sg_cpu->iowait_boost_max;
> - if (*util * boost_max < *max * boost_util) {
> - *util = boost_util;
> - *max = boost_max;
> - }
> + boost = (sg_cpu->iowait_boost * max) >> SCHED_CAPACITY_SHIFT;
> + return max(boost, util);
> }
>
> #ifdef CONFIG_NO_HZ_COMMON
> @@ -460,7 +448,7 @@ static void sugov_update_single(struct update_util_data
> *hook, u64 time,
>
> util = sugov_get_util(sg_cpu);
> max = sg_cpu->max;
> - sugov_iowait_apply(sg_cpu, time, &util, &max);
> + util = sugov_iowait_apply(sg_cpu, time, util, max);
> next_f = get_next_freq(sg_policy, util, max);
> /*
> * Do not reduce the frequency if the CPU has not been idle
> @@ -500,7 +488,7 @@ static unsigned int sugov_next_freq_shared(struct
> sugov_cpu *sg_cpu, u64 time)
>
> j_util = sugov_get_util(j_sg_cpu);
> j_max = j_sg_cpu->max;
> - sugov_iowait_apply(j_sg_cpu, time, &j_util, &j_max);
> + j_util = sugov_iowait_apply(j_sg_cpu, time, j_util, j_max);
>
> if (j_util * max > j_max * util) {
> util = j_util;
> @@ -837,7 +825,9 @@ static int sugov_start(struct cpufreq_policy *policy)
> memset(sg_cpu, 0, sizeof(*sg_cpu));
> sg_cpu->cpu = cpu;
> sg_cpu->sg_policy = sg_policy;
> - sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
> + sg_cpu->min =
> + (SCHED_CAPACITY_SCALE * policy->cpuinfo.min_freq) /
> + policy->cpuinfo.max_freq;
> }
>
> for_each_cpu(cpu, policy->cpus) {
>
