Re: [PATCH v3 1/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter

[Patrick Bellasi <patrick.bellasi@arm.com>]

On 30-Nov 17:02, Juri Lelli wrote:
> On 30/11/17 15:41, Patrick Bellasi wrote:
> > On 30-Nov 14:12, Juri Lelli wrote:
> > > Hi,
> > > 
> > > On 30/11/17 11:47, Patrick Bellasi wrote:
> > > 
> > > [...]
> > > 
> > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > index 2f52ec0f1539..67339ccb5595 100644
> > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > @@ -347,6 +347,12 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> > > >  
> > > >  	sg_cpu->util = util;
> > > >  	sg_cpu->max = max;
> > > > +
> > > > +	/* CPU is entering IDLE, reset flags without triggering an update */
> > > > +	if (unlikely(flags & SCHED_CPUFREQ_IDLE)) {
> > > > +		sg_cpu->flags = 0;
> > > > +		goto done;
> > > > +	}
> > > 
> > > Looks good for now. I'm just thinking that we will happen for DL, as a
> > > CPU that still "has" a sleeping task is not going to be really idle
> > > until the 0-lag time.
> > 
> > AFAIU, for the time being, DL already cannot really rely on this flag
> > for its behaviors to be correct. Indeed, flags are reset as soon as
> > a FAIR task wakes up and it's enqueued.
> 
> Right, and your flags ORing patch should help with this.
> 
> > 
> > Only once your DL integration patches are in, we do not depends on
> > flags anymore since DL will report a ceratain utilization up to the
> > 0-lag time, isn't it?
> 
> Utilization won't decrease until 0-lag time, correct.

Then IMO with your DL patches the DL class don't need the flags
anymore since schedutil will know (and account) for the
utlization required by the DL tasks. Isn't it?

> I was just wondering if resetting flags before that time (when a CPU
> enters idle) might be an issue.

If the above is correct, then flags will be used only for the RT class (and
IO boosting)... and thus this patch will still be useful as it is now:
meaning that once the idle task is selected we do not care anymore
about RT and IOBoosting (only).

> > If that's the case, I would say that the flags will be used only to
> > jump to the max OPP for RT tasks. Thus, this patch should still be valid.
> > 
> > > I guess we could move this at that point in time?
> > 
> > Not sure what you mean here. Right now the new SCHED_CPUFREQ_IDLE flag
> > is notified only by idle tasks. That's the only code path where we are
> > sure the CPU is entering IDLE.
> > 
> 
> W.r.t. the possible issue above, I was thinking that we might want to
> reset flags at 0-lag time for DL (if CPU is still idle). Anyway, two
> distinct set of patches. Who gets in last will have to ponder the thing
> a little bit more. :)

Perhaps I'm still a bit confused but, to me, it seems that with your
patches we completely fix DL but we still can use this exact same
patch just for RT tasks.

> Best,
> 
> Juri

-- 
#include <best/regards.h>

Patrick Bellasi