Messages in this thread | | | From | Suren Baghdasaryan <> | Date | Tue, 11 Sep 2018 12:25:28 -0700 | Subject | Re: [PATCH v4 11/16] sched/core: uclamp: add system default clamps |
| |
On Tue, Sep 11, 2018 at 9:46 AM, Patrick Bellasi <patrick.bellasi@arm.com> wrote: > On 10-Sep 09:20, Suren Baghdasaryan wrote: >> On Tue, Aug 28, 2018 at 6:53 AM, Patrick Bellasi >> <patrick.bellasi@arm.com> wrote: > > [...] > >> > @@ -1509,12 +1633,17 @@ static void __init init_uclamp(void) >> > uc_se->group_id = UCLAMP_NOT_VALID; >> > uclamp_group_get(NULL, clamp_id, 0, uc_se, >> > uclamp_none(clamp_id)); >> > + /* >> > + * By default we do not want task-specific clamp values, >> > + * so that system default values apply. >> > + */ >> > + uc_se->value = UCLAMP_NOT_VALID; >> > >> > #ifdef CONFIG_UCLAMP_TASK_GROUP >> > /* Init root TG's clamp group */ >> > uc_se = &root_task_group.uclamp[clamp_id]; >> > >> > - uc_se->effective.value = uclamp_none(clamp_id); >> > + uc_se->effective.value = uclamp_none(UCLAMP_MAX); >> >> Both clamps are initialized with 1023 because children can go lower >> but can't go higher? Comment might be helpful. > > Yes, that's because with CGroups we set the max allowed value, which > is also the one used for a clamp IFF: > - the task is not part of a more restrictive group > - the task has not a more restrictive task specific value > > I'll improve this comment on the next respin. > >> I saw this pattern of using uclamp_none(UCLAMP_MAX) for both clamps in >> couple places. > > The other place is to define / initialize "uclamp_default_perf", which > is the default clamps used for RT tasks, introduce by the last patch: > > https://lore.kernel.org/lkml/20180828135324.21976-17-patrick.bellasi@arm.com/ > > So, RT tasks and root task group are the only two exceptions for > which, by default, we want a maximum boosting. > >> Maybe would be better to have smth like: >> >> static inline int tg_uclamp_none(int clamp_id) { >> /* TG's min and max clamps default to SCHED_CAPACITY_SCALE to >> allow children to tighten the restriction */ >> return SCHED_CAPACITY_SCALE; >> } >> >> and use tg_uclamp_none(clamp_id) instead of uclamp_none(UCLAMP_MAX)? >> Functionally the same but much more readable. > > Not entirely convinced, maybe because of the name you suggest: it > cannot contain tg, because it applies also to RT tasks when TG are not > in use. > > Maybe something like: uclamp_max_boost(clamp_id) could work instead ?
Sounds good to me.
> > It will make more explicit that the configuration will maps into a: > > util.min = util.max = SCHED_CAPACITY_SCALE > > Cheers, > Patrick > > -- > #include <best/regards.h> > > Patrick Bellasi
| |