On Tue, Aug 06, 2019 at 17:11:34 +0100, Michal Koutný wrote...
> 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.)
Right, will move the uclamp_mutex introduction in this patch instead of
in the following one.
>> +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).)
So, looks like that since we are in the context of a file operation,
all the cgroup's attribute read/write functions are implicitly save.
IOW, we don't need an RCU lock since the TG data structures are granted
to be always available till the end of the read/write operation.
That seems to make sense... I'm wondering if keeping the RCU look is
still a precaution for possible future code/assumption changes or just
an unnecessary overhead?
>> @@ -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)?
Not sure to get what you mean here: I'm currently exposing uclamp to
both v1 and v2 hierarchies.
Best,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
