cpufreq: schedutil: Update last_update from sugov_set_iowait_boost()

sg_cpu->last_update is always updated right after we call
sugov_set_iowait_boost() and its better to update it from that routine
itself. This makes it more readable.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 kernel/sched/cpufreq_schedutil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.13.0.71.gd7076ec9c9cb

Hi Viresh,

On 18/07/17 10:24, Viresh Kumar wrote:
> sg_cpu->last_update is always updated right after we call

> sugov_set_iowait_boost() and its better to update it from that routine

> itself. This makes it more readable.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  kernel/sched/cpufreq_schedutil.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> index 29a397067ffa..63557b9f36b6 100644

> --- a/kernel/sched/cpufreq_schedutil.c

> +++ b/kernel/sched/cpufreq_schedutil.c

> @@ -177,6 +177,8 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,

>  		if (delta_ns > TICK_NSEC)

>  			sg_cpu->iowait_boost = 0;

>  	}

> +

> +	sg_cpu->last_update = time;

>  }

>  

>  static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,

> @@ -219,7 +221,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

>  	bool busy;

>  

>  	sugov_set_iowait_boost(sg_cpu, time, flags);

> -	sg_cpu->last_update = time;

>  

>  	if (!sugov_should_update_freq(sg_policy, time))

>  		return;

> @@ -299,7 +300,6 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,

>  	sg_cpu->flags = flags;

>  

>  	sugov_set_iowait_boost(sg_cpu, time, flags);

> -	sg_cpu->last_update = time;


It actually belongs here, IMHO. We update other fields (util, max,
flags) before looking at iowait. Why hiding the time update into a
function dealing with only one of such fields?

Best,

- Juri

On 18-07-17, 12:20, Juri Lelli wrote:
> Hi Viresh,

> 

> On 18/07/17 10:24, Viresh Kumar wrote:

> > sg_cpu->last_update is always updated right after we call

> > sugov_set_iowait_boost() and its better to update it from that routine

> > itself. This makes it more readable.

> > 

> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > ---

> >  kernel/sched/cpufreq_schedutil.c | 4 ++--

> >  1 file changed, 2 insertions(+), 2 deletions(-)

> > 

> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> > index 29a397067ffa..63557b9f36b6 100644

> > --- a/kernel/sched/cpufreq_schedutil.c

> > +++ b/kernel/sched/cpufreq_schedutil.c

> > @@ -177,6 +177,8 @@ static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,

> >  		if (delta_ns > TICK_NSEC)

> >  			sg_cpu->iowait_boost = 0;

> >  	}

> > +

> > +	sg_cpu->last_update = time;

> >  }

> >  

> >  static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long *util,

> > @@ -219,7 +221,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

> >  	bool busy;

> >  

> >  	sugov_set_iowait_boost(sg_cpu, time, flags);

> > -	sg_cpu->last_update = time;

> >  

> >  	if (!sugov_should_update_freq(sg_policy, time))

> >  		return;

> > @@ -299,7 +300,6 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,

> >  	sg_cpu->flags = flags;

> >  

> >  	sugov_set_iowait_boost(sg_cpu, time, flags);

> > -	sg_cpu->last_update = time;

> 

> It actually belongs here, IMHO. We update other fields (util, max,

> flags)


Yeah, because they have bigger roles and aren't specific to iowait
boost.

> before looking at iowait. Why hiding the time update into a

> function dealing with only one of such fields?


But last_update is very much specific to iowait_boost only and so it
should be updated from sugov_set_iowait_boost() IMHO.

-- 
viresh

On 18/07/17 16:55, Viresh Kumar wrote:
> On 18-07-17, 12:20, Juri Lelli wrote:

> > Hi Viresh,

> > 

> > On 18/07/17 10:24, Viresh Kumar wrote:


[...]

> > 

> > It actually belongs here, IMHO. We update other fields (util, max,

> > flags)

> 

> Yeah, because they have bigger roles and aren't specific to iowait

> boost.

> 

> > before looking at iowait. Why hiding the time update into a

> > function dealing with only one of such fields?

> 

> But last_update is very much specific to iowait_boost only and so it

> should be updated from sugov_set_iowait_boost() IMHO.

> 


Huh, I see you point. However, I'm using it also to deal with stale CFS
util values in my patches [1]. Sure, not mainline yet, but I guess I'll
have to sort of revert your proposed change for my next version. Or
think of a better idea. :)

Thanks,

- Juri

[1] http://marc.info/?l=linux-kernel&m=149924526528298&w=2

On Tuesday, July 18, 2017 12:35:52 PM Juri Lelli wrote:
> On 18/07/17 16:55, Viresh Kumar wrote:

> > On 18-07-17, 12:20, Juri Lelli wrote:

> > > Hi Viresh,

> > > 

> > > On 18/07/17 10:24, Viresh Kumar wrote:

> 

> [...]

> 

> > > 

> > > It actually belongs here, IMHO. We update other fields (util, max,

> > > flags)

> > 

> > Yeah, because they have bigger roles and aren't specific to iowait

> > boost.

> > 

> > > before looking at iowait. Why hiding the time update into a

> > > function dealing with only one of such fields?

> > 

> > But last_update is very much specific to iowait_boost only and so it

> > should be updated from sugov_set_iowait_boost() IMHO.

> > 

> 

> Huh, I see you point. However, I'm using it also to deal with stale CFS

> util values in my patches [1]. Sure, not mainline yet, but I guess I'll

> have to sort of revert your proposed change for my next version. Or

> think of a better idea. :)


Yes, so I'd prefer to prioritize your patches.

The patch from Viresh is purely cosmetic AFAICS and it really can wait.

Thanks,
Rafael