Message ID | 20171130114723.29210-3-patrick.bellasi@arm.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Hi, On 30/11/17 11:47, Patrick Bellasi wrote: [...] > @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > unsigned long util, max; > unsigned int next_f; > + bool rt_mode; > > sugov_get_util(&util, &max, sg_cpu->cpu); > > @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > sg_cpu->flags = 0; > goto done; > } > - sg_cpu->flags = flags; > + > + /* > + * While RT/DL tasks are running we do not want FAIR tasks to > + * overwrite this CPU's flags, still we can update utilization and > + * frequency (if required/possible) to be fair with these tasks. > + */ > + rt_mode = task_has_dl_policy(current) || > + task_has_rt_policy(current) || > + (flags & SCHED_CPUFREQ_RT_DL); > + if (rt_mode) > + sg_cpu->flags |= flags; > + else > + sg_cpu->flags = flags; > > sugov_set_iowait_boost(sg_cpu, time, flags); > sg_cpu->last_update = time; > > if (sugov_should_update_freq(sg_policy, time)) { > - if (flags & SCHED_CPUFREQ_RT_DL) > - next_f = sg_policy->policy->cpuinfo.max_freq; > - else > - next_f = sugov_next_freq_shared(sg_cpu, time); > - > + next_f = rt_mode > + ? sg_policy->policy->cpuinfo.max_freq > + : sugov_next_freq_shared(sg_cpu, time); > sugov_update_commit(sg_policy, time, next_f); Aren't we already at max_freq at this point (since an RT/DL task is running)? Do we need to trigger a frequency update? Best, Juri
On 30-Nov 14:17, Juri Lelli wrote: > Hi, > > On 30/11/17 11:47, Patrick Bellasi wrote: > > [...] > > > @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > > unsigned long util, max; > > unsigned int next_f; > > + bool rt_mode; > > > > sugov_get_util(&util, &max, sg_cpu->cpu); > > > > @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > sg_cpu->flags = 0; > > goto done; > > } > > - sg_cpu->flags = flags; > > + > > + /* > > + * While RT/DL tasks are running we do not want FAIR tasks to > > + * overwrite this CPU's flags, still we can update utilization and > > + * frequency (if required/possible) to be fair with these tasks. > > + */ > > + rt_mode = task_has_dl_policy(current) || > > + task_has_rt_policy(current) || > > + (flags & SCHED_CPUFREQ_RT_DL); > > + if (rt_mode) > > + sg_cpu->flags |= flags; > > + else > > + sg_cpu->flags = flags; > > > > sugov_set_iowait_boost(sg_cpu, time, flags); > > sg_cpu->last_update = time; > > > > if (sugov_should_update_freq(sg_policy, time)) { > > - if (flags & SCHED_CPUFREQ_RT_DL) > > - next_f = sg_policy->policy->cpuinfo.max_freq; > > - else > > - next_f = sugov_next_freq_shared(sg_cpu, time); > > - > > + next_f = rt_mode > > + ? sg_policy->policy->cpuinfo.max_freq > > + : sugov_next_freq_shared(sg_cpu, time); > > sugov_update_commit(sg_policy, time, next_f); > > Aren't we already at max_freq at this point (since an RT/DL task is > running)? Do we need to trigger a frequency update? I think that's required to cover the first time we enter rt_mode in order to jump to max OPP. If we are already at max OPP, sugov_update_commit() will just bail out without doing anything. Am I missing something? > > Best, > > Juri
On 30/11/17 15:45, Patrick Bellasi wrote: > On 30-Nov 14:17, Juri Lelli wrote: > > Hi, > > > > On 30/11/17 11:47, Patrick Bellasi wrote: > > > > [...] > > > > > @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > > > unsigned long util, max; > > > unsigned int next_f; > > > + bool rt_mode; > > > > > > sugov_get_util(&util, &max, sg_cpu->cpu); > > > > > > @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > > sg_cpu->flags = 0; > > > goto done; > > > } > > > - sg_cpu->flags = flags; > > > + > > > + /* > > > + * While RT/DL tasks are running we do not want FAIR tasks to > > > + * overwrite this CPU's flags, still we can update utilization and > > > + * frequency (if required/possible) to be fair with these tasks. > > > + */ > > > + rt_mode = task_has_dl_policy(current) || > > > + task_has_rt_policy(current) || > > > + (flags & SCHED_CPUFREQ_RT_DL); > > > + if (rt_mode) > > > + sg_cpu->flags |= flags; > > > + else > > > + sg_cpu->flags = flags; > > > > > > sugov_set_iowait_boost(sg_cpu, time, flags); > > > sg_cpu->last_update = time; > > > > > > if (sugov_should_update_freq(sg_policy, time)) { > > > - if (flags & SCHED_CPUFREQ_RT_DL) > > > - next_f = sg_policy->policy->cpuinfo.max_freq; > > > - else > > > - next_f = sugov_next_freq_shared(sg_cpu, time); > > > - > > > + next_f = rt_mode > > > + ? sg_policy->policy->cpuinfo.max_freq > > > + : sugov_next_freq_shared(sg_cpu, time); > > > sugov_update_commit(sg_policy, time, next_f); > > > > Aren't we already at max_freq at this point (since an RT/DL task is > > running)? Do we need to trigger a frequency update? > > I think that's required to cover the first time we enter rt_mode in > order to jump to max OPP. > > If we are already at max OPP, sugov_update_commit() will just bail out > without doing anything. Indeed! Best, Juri
On 30-11-17, 11:47, Patrick Bellasi wrote: > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 67339ccb5595..448f49de5335 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -262,6 +262,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > struct cpufreq_policy *policy = sg_policy->policy; > unsigned long util, max; > unsigned int next_f; > + bool rt_mode; > bool busy; > > sugov_set_iowait_boost(sg_cpu, time, flags); > @@ -272,7 +273,15 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > > busy = sugov_cpu_is_busy(sg_cpu); > > - if (flags & SCHED_CPUFREQ_RT_DL) { > + /* > + * While RT/DL tasks are running we do not want FAIR tasks to > + * overvrite this CPU's flags, still we can update utilization and > + * frequency (if required/possible) to be fair with these tasks. > + */ > + rt_mode = task_has_dl_policy(current) || > + task_has_rt_policy(current) || > + (flags & SCHED_CPUFREQ_RT_DL); > + if (rt_mode) { > next_f = policy->cpuinfo.max_freq; > } else { > sugov_get_util(&util, &max, sg_cpu->cpu); > @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > unsigned long util, max; > unsigned int next_f; > + bool rt_mode; > > sugov_get_util(&util, &max, sg_cpu->cpu); > > @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > sg_cpu->flags = 0; > goto done; > } > - sg_cpu->flags = flags; > + > + /* > + * While RT/DL tasks are running we do not want FAIR tasks to > + * overwrite this CPU's flags, still we can update utilization and > + * frequency (if required/possible) to be fair with these tasks. > + */ > + rt_mode = task_has_dl_policy(current) || > + task_has_rt_policy(current) || > + (flags & SCHED_CPUFREQ_RT_DL); > + if (rt_mode) > + sg_cpu->flags |= flags; > + else > + sg_cpu->flags = flags; > > sugov_set_iowait_boost(sg_cpu, time, flags); > sg_cpu->last_update = time; > > if (sugov_should_update_freq(sg_policy, time)) { > - if (flags & SCHED_CPUFREQ_RT_DL) > - next_f = sg_policy->policy->cpuinfo.max_freq; > - else > - next_f = sugov_next_freq_shared(sg_cpu, time); > - > + next_f = rt_mode > + ? sg_policy->policy->cpuinfo.max_freq > + : sugov_next_freq_shared(sg_cpu, time); > sugov_update_commit(sg_policy, time, next_f); > } Same here. There are pending comments from V2 which no one objected to and I was looking to see those modifications here.
On 07-Dec 10:35, Viresh Kumar wrote: > On 30-11-17, 11:47, Patrick Bellasi wrote: > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 67339ccb5595..448f49de5335 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -262,6 +262,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > > struct cpufreq_policy *policy = sg_policy->policy; > > unsigned long util, max; > > unsigned int next_f; > > + bool rt_mode; > > bool busy; > > > > sugov_set_iowait_boost(sg_cpu, time, flags); > > @@ -272,7 +273,15 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > > > > busy = sugov_cpu_is_busy(sg_cpu); > > > > - if (flags & SCHED_CPUFREQ_RT_DL) { > > + /* > > + * While RT/DL tasks are running we do not want FAIR tasks to > > + * overvrite this CPU's flags, still we can update utilization and > > + * frequency (if required/possible) to be fair with these tasks. > > + */ > > + rt_mode = task_has_dl_policy(current) || > > + task_has_rt_policy(current) || > > + (flags & SCHED_CPUFREQ_RT_DL); > > + if (rt_mode) { > > next_f = policy->cpuinfo.max_freq; > > } else { > > sugov_get_util(&util, &max, sg_cpu->cpu); > > @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > > unsigned long util, max; > > unsigned int next_f; > > + bool rt_mode; > > > > sugov_get_util(&util, &max, sg_cpu->cpu); > > > > @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > sg_cpu->flags = 0; > > goto done; > > } > > - sg_cpu->flags = flags; > > + > > + /* > > + * While RT/DL tasks are running we do not want FAIR tasks to > > + * overwrite this CPU's flags, still we can update utilization and > > + * frequency (if required/possible) to be fair with these tasks. > > + */ > > + rt_mode = task_has_dl_policy(current) || > > + task_has_rt_policy(current) || > > + (flags & SCHED_CPUFREQ_RT_DL); > > + if (rt_mode) > > + sg_cpu->flags |= flags; > > + else > > + sg_cpu->flags = flags; > > > > sugov_set_iowait_boost(sg_cpu, time, flags); > > sg_cpu->last_update = time; > > > > if (sugov_should_update_freq(sg_policy, time)) { > > - if (flags & SCHED_CPUFREQ_RT_DL) > > - next_f = sg_policy->policy->cpuinfo.max_freq; > > - else > > - next_f = sugov_next_freq_shared(sg_cpu, time); > > - > > + next_f = rt_mode > > + ? sg_policy->policy->cpuinfo.max_freq > > + : sugov_next_freq_shared(sg_cpu, time); > > sugov_update_commit(sg_policy, time, next_f); > > } > > Same here. There are pending comments from V2 which no one objected to > and I was looking to see those modifications here. So, your proposal here was actually to add additional flags to clear the RT and DL ones. My past comment was instead that we never had a "clear bit" semantics for flags updates. However, it seems that the most common optinion was that we should try to add such flags. Thus, I think I have to refresh this patch by adding in the new flags as you proposed and give it a try.
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 67339ccb5595..448f49de5335 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -262,6 +262,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, struct cpufreq_policy *policy = sg_policy->policy; unsigned long util, max; unsigned int next_f; + bool rt_mode; bool busy; sugov_set_iowait_boost(sg_cpu, time, flags); @@ -272,7 +273,15 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, busy = sugov_cpu_is_busy(sg_cpu); - if (flags & SCHED_CPUFREQ_RT_DL) { + /* + * While RT/DL tasks are running we do not want FAIR tasks to + * overvrite this CPU's flags, still we can update utilization and + * frequency (if required/possible) to be fair with these tasks. + */ + rt_mode = task_has_dl_policy(current) || + task_has_rt_policy(current) || + (flags & SCHED_CPUFREQ_RT_DL); + if (rt_mode) { next_f = policy->cpuinfo.max_freq; } else { sugov_get_util(&util, &max, sg_cpu->cpu); @@ -340,6 +349,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, struct sugov_policy *sg_policy = sg_cpu->sg_policy; unsigned long util, max; unsigned int next_f; + bool rt_mode; sugov_get_util(&util, &max, sg_cpu->cpu); @@ -353,17 +363,27 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, sg_cpu->flags = 0; goto done; } - sg_cpu->flags = flags; + + /* + * While RT/DL tasks are running we do not want FAIR tasks to + * overwrite this CPU's flags, still we can update utilization and + * frequency (if required/possible) to be fair with these tasks. + */ + rt_mode = task_has_dl_policy(current) || + task_has_rt_policy(current) || + (flags & SCHED_CPUFREQ_RT_DL); + if (rt_mode) + sg_cpu->flags |= flags; + else + sg_cpu->flags = flags; sugov_set_iowait_boost(sg_cpu, time, flags); sg_cpu->last_update = time; if (sugov_should_update_freq(sg_policy, time)) { - if (flags & SCHED_CPUFREQ_RT_DL) - next_f = sg_policy->policy->cpuinfo.max_freq; - else - next_f = sugov_next_freq_shared(sg_cpu, time); - + next_f = rt_mode + ? sg_policy->policy->cpuinfo.max_freq + : sugov_next_freq_shared(sg_cpu, time); sugov_update_commit(sg_policy, time, next_f); }