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. 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 being said, I am not strongly against the fine grained approach if that's what Yun wants now or what you both prefer. I just think the name of the flag needs to change to be more explicit too then. It'd be good to hear what others think. Cheers -- Qais Yousef
