Messages in this thread | | | Date | Mon, 26 Oct 2020 23:45:38 +0800 | From | Yun Hsiang <> | Subject | Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp |
| |
Hi Dietmar,
On Mon, Oct 26, 2020 at 10:47:11AM +0100, Dietmar Eggemann wrote: > On 25/10/2020 08:36, Yun Hsiang wrote: > > If the user wants to stop controlling uclamp and let the task inherit > > the value from the group, we need a method to reset. > > > > Add SCHED_FLAG_UTIL_CLAMP_RESET flag to allow the user to reset uclamp via > > sched_setattr syscall. > > > > The policy is > > _CLAMP_RESET => reset both min and max > > _CLAMP_RESET | _CLAMP_MIN => reset min value > > _CLAMP_RESET | _CLAMP_MAX => reset max value > > _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max > > > > Signed-off-by: Yun Hsiang <hsiang023167@gmail.com> > > [...] > > > @@ -1451,7 +1464,8 @@ static void __setscheduler_uclamp(struct task_struct *p, > > struct uclamp_se *uc_se = &p->uclamp_req[clamp_id]; > > > > /* Keep using defined clamps across class changes */ > > - if (uc_se->user_defined) > > + if (flags != SCHED_FLAG_UTIL_CLAMP_RESET && > > + uc_se->user_defined) > > continue; > > With: > > (1) _CLAMP_RESET => reset both min and max > (2) _CLAMP_RESET | _CLAMP_MIN => reset min value > (3) _CLAMP_RESET | _CLAMP_MAX => reset max value > (4) _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max > > If you reset an RT task with (1) you don't reset its uclamp.min value. > > __uclamp_update_util_min_rt_default(p) doesn't know about > SCHED_FLAG_UTIL_CLAMP_RESET. It only knows user_defined and will bail early. >
Sorry I didn't notice __uclamp_update_util_min_rt_default will return directly if user_defined is set, I'll fix it.
> [...] > > > - if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP))) > > + if (likely(!flags || flags == SCHED_FLAG_UTIL_CLAMP_RESET)) > > return; > > > > - if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) { > > + if (flags & SCHED_FLAG_UTIL_CLAMP_MIN) { > > + if (reset) { > > + clamp_value = __default_uclamp_value(p, UCLAMP_MIN); > > + user_defined = false; > > + } else { > > + clamp_value = attr->sched_util_min; > > + user_defined = true; > > + } > > Why do you reset for (1) in the for_each_clamp_id(clamp_id) loop and for > (2)-(4) in the if (flags & SCHED_FLAG_UTIL_CLAMP_MXX) condition? > > You could reset (1)-(4) in the for_each_clamp_id(clamp_id) loop? In this > case you wouldn't need __default_uclamp_value().
Do you mean adding these code in for_each_clamp_id(clamp_id) loop?
if (clamp_id == UCLAMP_MIN) { if (flags == SCHED_FLAG_UTIL_CLAMP_RESET || (reset && (flags || SCHED_FLAG_UTIL_CLAMP_MIN)) || !se->user_defined) { if (task_rt(p)) { clamp_value = sysctl_sched_uclamp_util_min_rt_default } else { clamp_value = uclamp_none(clamp_id); } } else continue; } /* similar code for UCLAMP_MAX */ ... uclamp_se_set(uc_se, clamp_value, false);
It seems more clear. If you think this one is better, I'll use this method and send patch V4.
> [...]
| |