On Tue, Oct 13, 2020 at 15:32:46 +0200, Qais Yousef <qais.you...@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 ;)