I Qais,
I see we are converging toward the final shape. :)
Function wise code looks ok to me now.
Lemme just point out few more remarks and possible nit-picks.
I guess at the end it's up to you to decide if you wanna follow up with
a v6 and to the maintainers to decide how picky they wanna be.
Otherwise, FWIW, feel free to consider this a LGTM.
Best,
Patrick
On Mon, May 11, 2020 at 17:40:52 +0200, Qais Yousef <qais.yousef@xxxxxxx> wrote...
[...]
> +static inline void uclamp_sync_util_min_rt_default(struct task_struct *p,
> + enum uclamp_id clamp_id)
> +{
> + unsigned int default_util_min = sysctl_sched_uclamp_util_min_rt_default;
> + struct uclamp_se *uc_se;
> +
> + /* Only sync for UCLAMP_MIN and RT tasks */
> + if (clamp_id != UCLAMP_MIN || !rt_task(p))
> + return;
> +
> + uc_se = &p->uclamp_req[UCLAMP_MIN];
I went back to v3 version, where this was done above:
https://lore.kernel.org/lkml/20200429113255.GA19464@xxxxxxxxxxxxxx/
Message-ID: 20200429113255.GA19464@xxxxxxxxxxxxxx
and still I don't see why we want to keep it after this first check?
IMO it's just not required and it makes to code a tiny uglier.
> +
> + /*
> + * Only sync if user didn't override the default request and the sysctl
> + * knob has changed.
> + */
> + if (uc_se->user_defined || uc_se->value == default_util_min)
> + return;
> +
nit-pick: the two comments above are stating the obvious.
> + uclamp_se_set(uc_se, default_util_min, false);
> +}
> +
> static inline struct uclamp_se
> uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
> {
> @@ -907,8 +949,13 @@ uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
> static inline struct uclamp_se
> uclamp_eff_get(struct task_struct *p, enum uclamp_id clamp_id)
> {
> - struct uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id);
> - struct uclamp_se uc_max = uclamp_default[clamp_id];
> + struct uclamp_se uc_req, uc_max;
> +
> + /* Sync up any change to sysctl_sched_uclamp_util_min_rt_default. */
same here: the comment is stating the obvious.
Maybe even just by using a different function name we better document
the code, e.g. uclamp_rt_restrict(p, clamp_id);
This will implicitly convey the purpose: RT tasks can be somehow
further restricted, i.e. in addition to the TG restriction following.
> + uclamp_sync_util_min_rt_default(p, clamp_id);
> +
> + uc_req = uclamp_tg_restrict(p, clamp_id);
> + uc_max = uclamp_default[clamp_id];
>
> /* System default restrictions always apply */
> if (unlikely(uc_req.value > uc_max.value))
[...]
