Messages in this thread | | | From | Patrick Bellasi <> | Subject | Re: [PATCH v2 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp | Date | Tue, 13 Oct 2020 18:52:03 +0200 |
| |
On Tue, Oct 13, 2020 at 15:32:46 +0200, Qais Yousef <qais.yousef@arm.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 ;)
| |