linux-kernel - Re: [PATCH v2 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87362ihxvw.derkling@matbug.net>
Date:   Tue, 13 Oct 2020 18:52:03 +0200
From:   Patrick Bellasi <patrick.bellasi@...bug.net>
To:     Qais Yousef <qais.yousef@....com>
Cc:     Yun Hsiang <hsiang023167@...il.com>, dietmar.eggemann@....com,
        peterz@...radead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET
 flag to reset uclamp

On Tue, Oct 13, 2020 at 15:32:46 +0200, Qais Yousef <qais.yousef@....com> wrote...

> On 10/13/20 13:46, Patrick Bellasi wrote:
>> > So IMO you just need a single SCHED_FLAG_UTIL_CLAMP_RESET that if set in the
>> > attr, you just execute that loop in __setscheduler_uclamp() + reset
>> > uc_se->user_defined.
>> >
>> > It should be invalid to pass the SCHED_FLAG_UTIL_CLAMP_RESET with
>> > SCHED_FLAG_UTIL_CLAMP_MIN/MAX. Both have contradictory meaning IMO.
>> > If user passes both we should return an EINVAL error.
>> 
>> Passing in  _CLAMP_RESET|_CLAMP_MIN will mean reset the min value while
>> keeping the max at whatever it is. I think there could be cases where
>> this support could be on hand.
>
> I am not convinced personally. I'm anxious about what this fine grained control
> means and how it should be used. I think less is more in this case and we can
> always relax the restriction (appropriately) later if it's *really* required.
>
> Particularly the fact that this user_defined is per uclamp_se and that it
> affects the cgroup behavior is implementation details this API shouldn't rely
> on.

The user_defined flag is an implementation details: true, but since the
beginning uclamp _always_ allowed a task to set only one of its clamp
values.

That's why we have UTIL_CLAMP_{MIN,MAX} as separate flags and all the
logic in place to set only one of the two.

> A generic RESET my uclamp settings makes more sense for me as a long term
> API to maintain.
>
> Actually maybe we should even go for a more explicit
> SCHED_FLAG_UTIL_CLAMP_INHERIT_CGROUP flag instead. If we decide to abandon the
> support for this feature in the future, at least we can make it return an error
> without affecting other functionality because of the implicit nature of
> SCHED_FLAG_UTIL_CLAMP_RESET means inherit cgroup value too.

That's not true and it's an even worst implementation detail what you
want to expose.

A task without a task specific clamp _always_ inherits the system
defaults. Resetting a task specific clamp already makes sense also
_without_ cgroups. It means: just do whatever the system allows you to
do.

Only if you are running with CGRoups enabled and the task happens to be
_not_ in the root group, the "CGroups inheritance" happens.
But that's exactly an internal detail a task should not care about.

> That being said, I am not strongly against the fine grained approach if that's
> what Yun wants now or what you both prefer.

It's not a fine grained approach, it's just adding a reset mechanism for
what uclamp already allows to do: setting min and max clamps
independently.

Regarding use cases, I also believe we have many more use cases of tasks
interested in setting/resetting just one clamp than tasks interested in
"fine grain" controlling both clamps at the same time.

> I just think the name of the flag needs to change to be more explicit
> too then.

I don't agree on that and, again, I see much more fine grained details and
internals exposure in what you propose compared to a single generic
_RESET flag.

> It'd be good to hear what others think.

I agree on that ;)