Messages in this thread | | | Date | Thu, 15 Oct 2020 00:15:25 +0800 | From | Yun Hsiang <> | Subject | Re: [PATCH v2 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp |
| |
On Tue, Oct 13, 2020 at 06:52:03PM +0200, Patrick Bellasi wrote:
Hi Patrick,
> > 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.
I agree that UTIL_CLAMP_{MIN,MAX} should be able to set separately. So the flag usage might be _CLAMP_RESET => ? _CLAMP_RESET | _CLAMP_MIN => reset min value _CLAMP_RESET | _CLAMP_MAX => reset max value _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max If user pass _CLAMP_RESET without _CLAMP_MIN or _CLAMP MAX, Should we reset both min & max value or return EINVAL error?
> > > > 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.
I prefer to use SCHED_FLAG_UTIL_CLAMP_RESET because cgroup inheritance is default behavior when task doesn't set it's uclamp.
> > > 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 ;) >
| |