Re: [PATCH v13 1/6] sched/core: uclamp: Extend CPU's cgroup controller

[Michal Koutný]

On Fri, Aug 02, 2019 at 10:08:48AM +0100, Patrick Bellasi <patrick.bellasi@xxxxxxx> wrote:
> +static ssize_t cpu_uclamp_write(struct kernfs_open_file *of, char *buf,
> +				size_t nbytes, loff_t off,
> +				enum uclamp_id clamp_id)
> +{
> +	struct uclamp_request req;
> +	struct task_group *tg;
> +
> +	req = capacity_from_percent(buf);
> +	if (req.ret)
> +		return req.ret;
> +
> +	rcu_read_lock();
This should be the uclamp_mutex.

(The compound results of the series is correct as the lock is introduced
in "sched/core: uclamp: Propagate parent clamps".
This is just for the happiness of cherry-pickers/bisectors.)

> +static inline void cpu_uclamp_print(struct seq_file *sf,
> +				    enum uclamp_id clamp_id)
> +{
> [...]
> +	rcu_read_lock();
> +	tg = css_tg(seq_css(sf));
> +	util_clamp = tg->uclamp_req[clamp_id].value;
> +	rcu_read_unlock();
Why is the rcu_read_lock() needed here? (I'm considering the comment in
of_css() that should apply here (and it seems that similar uses in other
seq_file handlers also skip this).)

> @@ -7369,6 +7506,20 @@ static struct cftype cpu_legacy_files[] = {
> [...]
> +		.name = "uclamp.min",
> [...]
> +		.name = "uclamp.max",
I don't see technical reasons why uclamp couldn't work on legacy
hierarchy and Tejun acked the series, despite that I'll ask -- should
the new attributes be exposed in v1 controller hierarchy (taking into
account the legacy API is sort of frozen and potential maintenance needs
spanning both hierarchies)?