On 10-04-18, 11:43, Patrick Bellasi wrote:
> On 05-Apr 15:28, Viresh Kumar wrote:
> What about this new version for the two functions,
> just compile tested:
> 
> ---8<---
> 
> static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
>                                  unsigned int flags)
> {
>       bool iowait = flags & SCHED_CPUFREQ_IOWAIT;
> 
>       /* Reset boost if the CPU appears to have been idle enough */
>       if (sg_cpu->iowait_boost) {
>               s64 delta_ns = time - sg_cpu->last_update;
> 
>               if (delta_ns > TICK_NSEC) {
>                       sg_cpu->iowait_boost = iowait
>                               ? sg_cpu->sg_policy->policy->min : 0;

Yeah, I see you are trying to optimize it a bit here but this makes
things more confusing I would say :)

I would just set iowait_boost to 0 and drop the return; from below and
let the code fall through and reach the end of this routine.

>                       sg_cpu->iowait_boost_pending = iowait;
>                       return;
>               }
>       }
> 
>       /* Boost only tasks waking up after IO */
>       if (!iowait)
>               return;
> 
>       /* Ensure IO boost doubles only one time at each frequency increase */
>       if (sg_cpu->iowait_boost_pending)
>               return;
>       sg_cpu->iowait_boost_pending = true;
> 
>       /* Double the IO boost at each frequency increase */
>       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;
>               return;
>       }
> 
>       /* At first wakeup after IO, start with minimum boost */
>       sg_cpu->iowait_boost = sg_cpu->sg_policy->policy->min;
> }
> 
> static void sugov_iowait_boost(struct sugov_cpu *sg_cpu,
>                              unsigned long *util, unsigned long *max)
> {
>       unsigned int boost_util, boost_max;
> 
>         /* No IOWait boost active */
>         if (!sg_cpu->iowait_boost)
>                 return;
> 
>       /* An IO waiting task has just woken up, use the boost value */
>       if (sg_cpu->iowait_boost_pending) {
>               sg_cpu->iowait_boost_pending = false;
>       } else {
>               /* Reduce the boost value otherwise */
>               sg_cpu->iowait_boost >>= 1;
>               if (sg_cpu->iowait_boost < sg_cpu->sg_policy->policy->min) {
>                       sg_cpu->iowait_boost = 0;
>                       return;
>               }
>       }
> 
>       boost_util = sg_cpu->iowait_boost;
>       boost_max = sg_cpu->iowait_boost_max;
> 
>       /*
>        * A CPU is boosted only if its current utilization is smaller then
>        * the current IO boost level.
>        */
>       if (*util * boost_max < *max * boost_util) {
>               *util = boost_util;
>               *max = boost_max;
>       }
> }

So this is quite different than what you proposed, it is only fixing
the existing problem which I pointed out to. Looks fine, not much
changed really from the current state of code.

-- 
viresh

Reply via email to