A few uclamp fixes

The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last active task to maintain the last uclamp.max and prevent blocked util from suddenly becoming visible.

However, there is an asymmetry in how the flag is set and cleared which can lead to having the flag set whilst there are active tasks on the rq. Specifically, the flag is cleared in the uclamp_rq_inc() path, which is called at enqueue time, but set in uclamp_rq_dec_id() which is called both when dequeueing a task _and_ in the update_uclamp_active() path. As a result, when both uclamp_rq_{dec,ind}_id() are called from update_uclamp_active(), the flag ends up being set but not cleared, hence leaving the runqueue in a broken state.

Fix this by setting the flag in the uclamp_rq_inc_id() path to ensure things remain symmetrical.

Fixes e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")

Reported-by Rick Yiu <[email protected]>

Reviewed-by Qais Yousef <[email protected]>

Signed-off-by Quentin Perret <[email protected]>

---
 kernel/sched/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

kernel/sched/core.c +1 -4

index 5226cc26a095..3b213402798e 100644
@@ -980,6 +980,7 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
 	if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
 		return;
 
+	rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
 	WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
 }
 

@@ -1252,10 +1253,6 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
 
 	for_each_clamp_id(clamp_id)
 		uclamp_rq_inc_id(rq, p, clamp_id);
-
-	/* Reset clamp idle holding when there is one RUNNABLE task */
-	if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
-		rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
 }
 
 static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)

Signature · 1 line

2.32.0.272.g935e593368-goog

Quoted 13 lines · Quentin Perret

The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last active task to maintain the last uclamp.max and prevent blocked util from suddenly becoming visible.

However, there is an asymmetry in how the flag is set and cleared which can lead to having the flag set whilst there are active tasks on the rq. Specifically, the flag is cleared in the uclamp_rq_inc() path, which is called at enqueue time, but set in uclamp_rq_dec_id() which is called both when dequeueing a task _and_ in the update_uclamp_active() path. As a result, when both uclamp_rq_{dec,ind}_id() are called from update_uclamp_active(), the flag ends up being set but not cleared, hence leaving the runqueue in a broken state.

Quoted 2 lines

Fix this by setting the flag in the uclamp_rq_inc_id() path to ensure things remain symmetrical.

The code you moved is neither in uclamp_rq_inc_id(), although uclamp_idle_reset() is called from there

nor does it _set_ the flag.

I'm thinking it's been a long warm day? ;-)

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Quentin Perret

The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last active task to maintain the last uclamp.max and prevent blocked util from suddenly becoming visible.

However, there is an asymmetry in how the flag is set and cleared which can lead to having the flag set whilst there are active tasks on the rq. Specifically, the flag is cleared in the uclamp_rq_inc() path, which is called at enqueue time, but set in uclamp_rq_dec_id() which is called both when dequeueing a task _and_ in the update_uclamp_active() path. As a result, when both uclamp_rq_{dec,ind}_id() are called from update_uclamp_active(), the flag ends up being set but not cleared, hence leaving the runqueue in a broken state.

Fix this by setting the flag in the uclamp_rq_inc_id() path to ensure things remain symmetrical.

Quoted 2 lines

The code you moved is neither in uclamp_rq_inc_id(), although uclamp_idle_reset() is called from there

Yep, that is what I was trying to say.

Quoted 1 line

nor does it _set_ the flag.

Ahem. That I don't have a good excuse for ...

Quoted 18 lines · Quentin Perret

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Quentin Perret

The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last active task to maintain the last uclamp.max and prevent blocked util from suddenly becoming visible.

However, there is an asymmetry in how the flag is set and cleared which can lead to having the flag set whilst there are active tasks on the rq. Specifically, the flag is cleared in the uclamp_rq_inc() path, which is called at enqueue time, but set in uclamp_rq_dec_id() which is called both when dequeueing a task _and_ in the update_uclamp_active() path. As a result, when both uclamp_rq_{dec,ind}_id() are called from update_uclamp_active(), the flag ends up being set but not cleared, hence leaving the runqueue in a broken state.

Fix this by setting the flag in the uclamp_rq_inc_id() path to ensure things remain symmetrical.

Quoted 8 lines

Quoted 2 lines

The code you moved is neither in uclamp_rq_inc_id(), although uclamp_idle_reset() is called from there

Yep, that is what I was trying to say.

Quoted 1 line

nor does it _set_ the flag.

Ahem. That I don't have a good excuse for ...

(A) dequeue -> set

(1) dequeue_task() -> uclamp_rq_dec() ->

(2) cpu_util_update_eff() -> ... -> uclamp_update_active() ->

uclamp_rq_dec_id()

    uclamp_rq_max_value()

        /* No tasks -- default clamp values */
        uclamp_idle_value() {

            if (clamp_id == UCLAMP_MAX)
                rq->uclamp_flags |= UCLAMP_FLAG_IDLE;  <-- set
        }

---

(B) enqueue -> clear

(1) enqueue_task() ->

uclamp_rq_inc() {

(2) cpu_util_update_eff() -> ... -> uclamp_update_active() ->

    uclamp_rq_inc_id() {

        uclamp_idle_reset() {
    						     <-- new clear
       }                                                     ^
    }                                                        |
                                                             |
    if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)                 |
        rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;       <-- old clear
}

---

uclamp_update_active()

    if (p->uclamp[clamp_id].active) {
        uclamp_rq_dec_id()            <-- (A2)
	uclamp_rq_inc_id()            <-- (B2)
    }

Is this existing asymmetry in setting the flag but not clearing it in uclamp_update_active() the only issue this patch fixes?

SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that the call must not touch scheduling parameters (nice or priority). This is particularly handy for uclamp when used in conjunction with SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only impacts uclamp values.

However, sched_setattr always checks whether the priorities and nice values passed in sched_attr are valid first, even if those never get used down the line. This is useless at best since userspace can trivially bypass this check to set the uclamp values by specifying low priorities. However, it is cumbersome to do so as there is no single expression of this that skips both RT and CFS checks at once. As such, userspace needs to query the task policy first with e.g. sched_getattr and then set sched_attr.sched_priority accordingly. This is racy and slower than a single call.

As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS is specified, simply inherit them in this case to match the policy inheritance of SCHED_FLAG_KEEP_POLICY.

Reported-by Wei Wang <[email protected]>

Signed-off-by Quentin Perret <[email protected]>

---
 kernel/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/core.c +0 -0

index 3b213402798e..1d4aedbbcf96 100644

Quoted 31 lines · Quentin Perret

SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that the call must not touch scheduling parameters (nice or priority). This is particularly handy for uclamp when used in conjunction with SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only impacts uclamp values.

However, sched_setattr always checks whether the priorities and nice values passed in sched_attr are valid first, even if those never get used down the line. This is useless at best since userspace can trivially bypass this check to set the uclamp values by specifying low priorities. However, it is cumbersome to do so as there is no single expression of this that skips both RT and CFS checks at once. As such, userspace needs to query the task policy first with e.g. sched_getattr and then set sched_attr.sched_priority accordingly. This is racy and slower than a single call.

As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS is specified, simply inherit them in this case to match the policy inheritance of SCHED_FLAG_KEEP_POLICY.

Reported-by Wei Wang <[email protected]>

Signed-off-by Quentin Perret <[email protected]>

---
 kernel/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/core.c +0 -0

index 3b213402798e..1d4aedbbcf96 100644

kernel/sched/core.c SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, · line 6594

Quoted 10 lines

kernel/sched/core.c +4 -0

@@ -6585,6 +6585,10 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	rcu_read_unlock();
 
 	if (likely(p)) {
+		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS) {
+			attr.sched_priority = p->rt_priority;
+			attr.sched_nice = task_nice(p);
+		}
 		retval = sched_setattr(p, &attr);
 		put_task_struct(p);
 	}

I don't like this much... afaict the KEEP_PARAMS clause in __setscheduler() also covers the DL params, and you 'forgot' to copy those.

Can't we short circuit the validation logic?

Quoted 47 lines · Peter Zijlstra

Quoted 31 lines · Quentin Perret

SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that the call must not touch scheduling parameters (nice or priority). This is particularly handy for uclamp when used in conjunction with SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only impacts uclamp values.

However, sched_setattr always checks whether the priorities and nice values passed in sched_attr are valid first, even if those never get used down the line. This is useless at best since userspace can trivially bypass this check to set the uclamp values by specifying low priorities. However, it is cumbersome to do so as there is no single expression of this that skips both RT and CFS checks at once. As such, userspace needs to query the task policy first with e.g. sched_getattr and then set sched_attr.sched_priority accordingly. This is racy and slower than a single call.

As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS is specified, simply inherit them in this case to match the policy inheritance of SCHED_FLAG_KEEP_POLICY.

Reported-by Wei Wang <[email protected]>

Signed-off-by Quentin Perret <[email protected]>

---
 kernel/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/core.c +0 -0

index 3b213402798e..1d4aedbbcf96 100644

kernel/sched/core.c SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, · line 6594

Quoted 10 lines

kernel/sched/core.c +4 -0

@@ -6585,6 +6585,10 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	rcu_read_unlock();
 
 	if (likely(p)) {
+		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS) {
+			attr.sched_priority = p->rt_priority;
+			attr.sched_nice = task_nice(p);
+		}
 		retval = sched_setattr(p, &attr);
 		put_task_struct(p);
 	}

I don't like this much... afaict the KEEP_PARAMS clause in __setscheduler() also covers the DL params, and you 'forgot' to copy those.

Quoted 1 line

Can't we short circuit the validation logic?

I think we can but I didn't like the look of it, because we end up sprinkling checks all over the place. KEEP_PARAMS doesn't imply KEEP_POLICY IIUC, and the policy and params checks are all mixed up.

But maybe that wants fixing too? I guess it could make sense to switch policies without touching the params in some cases (e.g switching between FIFO and RR, or BATCH and NORMAL), but I'm not sure what that would mean for cross-sched_class transitions.

Quoted 47 lines · Peter Zijlstra

Quoted 31 lines · Quentin Perret

SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that the call must not touch scheduling parameters (nice or priority). This is particularly handy for uclamp when used in conjunction with SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only impacts uclamp values.

However, sched_setattr always checks whether the priorities and nice values passed in sched_attr are valid first, even if those never get used down the line. This is useless at best since userspace can trivially bypass this check to set the uclamp values by specifying low priorities. However, it is cumbersome to do so as there is no single expression of this that skips both RT and CFS checks at once. As such, userspace needs to query the task policy first with e.g. sched_getattr and then set sched_attr.sched_priority accordingly. This is racy and slower than a single call.

As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS is specified, simply inherit them in this case to match the policy inheritance of SCHED_FLAG_KEEP_POLICY.

Reported-by Wei Wang <[email protected]>

Signed-off-by Quentin Perret <[email protected]>

---
 kernel/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/core.c +0 -0

index 3b213402798e..1d4aedbbcf96 100644

kernel/sched/core.c SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, · line 6594

Quoted 10 lines

kernel/sched/core.c +4 -0

@@ -6585,6 +6585,10 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	rcu_read_unlock();
 
 	if (likely(p)) {
+		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS) {
+			attr.sched_priority = p->rt_priority;
+			attr.sched_nice = task_nice(p);
+		}
 		retval = sched_setattr(p, &attr);
 		put_task_struct(p);
 	}

I don't like this much... afaict the KEEP_PARAMS clause in __setscheduler() also covers the DL params, and you 'forgot' to copy those.

Quoted 1 line

Can't we short circuit the validation logic?

I think we can but I didn't like the look of it, because we end up sprinkling checks all over the place. KEEP_PARAMS doesn't imply KEEP_POLICY IIUC, and the policy and params checks are all mixed up.

But maybe that wants fixing too?

I guess it could make sense to switch
policies without touching the params in some cases (e.g switching between FIFO and RR, or BATCH and NORMAL), but I'm not sure what that would mean for cross-sched_class transitions.

There is currently nothing preventing tasks from changing their per-task clamp values in anyway that they like. The rationale is probably that system administrators are still able to limit those clamps thanks to the cgroup interface. However, this causes pain in a system where both per-task and per-cgroup clamp values are expected to be under the control of core system components (as is the case for Android).

To fix this, let's require CAP_SYS_NICE to increase per-task clamp values. This allows unprivileged tasks to lower their requests, but not increase them, which is consistent with the existing behaviour for nice values.

Quoted 11 lines · Quentin Perret

There is currently nothing preventing tasks from changing their per-task clamp values in anyway that they like. The rationale is probably that system administrators are still able to limit those clamps thanks to the cgroup interface. However, this causes pain in a system where both per-task and per-cgroup clamp values are expected to be under the control of core system components (as is the case for Android).

To fix this, let's require CAP_SYS_NICE to increase per-task clamp values. This allows unprivileged tasks to lower their requests, but not increase them, which is consistent with the existing behaviour for nice values.

Hmmm. I'm not in favour of this.

So uclamp is a performance and power management mechanism, it has no impact on fairness AFAICT, so it being a privileged operation doesn't make sense.

We had a thought about this in the past and we didn't think there's any harm if a task (app) wants to self manage. Yes a task could ask to run at max performance and waste power, but anyone can generate a busy loop and waste power too.

Now that doesn't mean your use case is not valid. I agree if there's a system wide framework that wants to explicitly manage performance and power of tasks via uclamp, then we can end up with 2 layers of controls overriding each others.

Would it make more sense to have a procfs/sysfs flag that is disabled by default that allows sys-admin to enforce a privileged uclamp access?

Something like

/proc/sys/kernel/sched_uclamp_privileged

I think both usage scenarios are valid and giving sys-admins the power to enforce a behavior makes more sense for me.

Hi Qais,

Quoted 23 lines · Qais Yousef

Quoted 11 lines · Quentin Perret

There is currently nothing preventing tasks from changing their per-task clamp values in anyway that they like. The rationale is probably that system administrators are still able to limit those clamps thanks to the cgroup interface. However, this causes pain in a system where both per-task and per-cgroup clamp values are expected to be under the control of core system components (as is the case for Android).

To fix this, let's require CAP_SYS_NICE to increase per-task clamp values. This allows unprivileged tasks to lower their requests, but not increase them, which is consistent with the existing behaviour for nice values.

Hmmm. I'm not in favour of this.

So uclamp is a performance and power management mechanism, it has no impact on fairness AFAICT, so it being a privileged operation doesn't make sense.

We had a thought about this in the past and we didn't think there's any harm if a task (app) wants to self manage. Yes a task could ask to run at max performance and waste power, but anyone can generate a busy loop and waste power too.

Quoted 4 lines

Now that doesn't mean your use case is not valid. I agree if there's a system wide framework that wants to explicitly manage performance and power of tasks via uclamp, then we can end up with 2 layers of controls overriding each others.

Right, that's the main issue. Also, the reality is that most of time the 'right' clamps are platform-dependent, so most userspace apps are simply not equipped to decide what their own clamps should be.

Quoted 7 lines

Would it make more sense to have a procfs/sysfs flag that is disabled by default that allows sys-admin to enforce a privileged uclamp access?

Something like

/proc/sys/kernel/sched_uclamp_privileged

Hmm, dunno, I'm not aware of anything else having a behaviour like that, so that feels a bit odd.

Quoted 3 lines

I think both usage scenarios are valid and giving sys-admins the power to enforce a behavior makes more sense for me.

Yes, I wouldn't mind something like that in general. I originally wanted to suggest introducing a dedicated capability for uclamp, but that felt a bit overkill. Now if others think this should be the way to go I'm happy to go implement it.

Hi Quentin

Quoted 26 lines · Quentin Perret

Hi Qais,

Quoted 23 lines · Qais Yousef

Quoted 11 lines · Quentin Perret

There is currently nothing preventing tasks from changing their per-task clamp values in anyway that they like. The rationale is probably that system administrators are still able to limit those clamps thanks to the cgroup interface. However, this causes pain in a system where both per-task and per-cgroup clamp values are expected to be under the control of core system components (as is the case for Android).

To fix this, let's require CAP_SYS_NICE to increase per-task clamp values. This allows unprivileged tasks to lower their requests, but not increase them, which is consistent with the existing behaviour for nice values.

Hmmm. I'm not in favour of this.

So uclamp is a performance and power management mechanism, it has no impact on fairness AFAICT, so it being a privileged operation doesn't make sense.

We had a thought about this in the past and we didn't think there's any harm if a task (app) wants to self manage. Yes a task could ask to run at max performance and waste power, but anyone can generate a busy loop and waste power too.

Quoted 8 lines

Quoted 4 lines

Now that doesn't mean your use case is not valid. I agree if there's a system wide framework that wants to explicitly manage performance and power of tasks via uclamp, then we can end up with 2 layers of controls overriding each others.

Right, that's the main issue. Also, the reality is that most of time the 'right' clamps are platform-dependent, so most userspace apps are simply not equipped to decide what their own clamps should be.

I'd argue this is true for both a framework or an app point of view. It depends on the application and how it would be used.

I can foresee for example and HTTP server wanting to use uclamp to guarantee a QoS target ie: X number of requests per second or a maximum of Y tail latency. The application can try to tune (calibrate) itself without having to have the whole system tuned or pumped on steroid.

Or a framework could manage this on behalf of the application. Both can use uclamp with a feedback loop to calibrate the perf requirement of the tasks to meet a given perf/power criteria.

If you want to do a static management, system framework would make more sense in this case, true.

Quoted 4 lines

Quoted 4 lines

Would it make more sense to have a procfs/sysfs flag that is disabled by default that allows sys-admin to enforce a privileged uclamp access?

Quoted 6 lines

Quoted 3 lines

Something like

/proc/sys/kernel/sched_uclamp_privileged

Hmm, dunno, I'm not aware of anything else having a behaviour like that, so that feels a bit odd.

I think /proc/sys/kernel/perf_event_paranoid falls into this category.

Quoted 8 lines

Quoted 3 lines

I think both usage scenarios are valid and giving sys-admins the power to enforce a behavior makes more sense for me.

Yes, I wouldn't mind something like that in general. I originally wanted to suggest introducing a dedicated capability for uclamp, but that felt a bit overkill. Now if others think this should be the way to go I'm happy to go implement it.

Would be good to hear what others think for sure :)

Quoted 41 lines · Qais Yousef

Hi Quentin

Quoted 26 lines · Quentin Perret

Hi Qais,

Quoted 23 lines · Qais Yousef

Quoted 11 lines · Quentin Perret

There is currently nothing preventing tasks from changing their per-task clamp values in anyway that they like. The rationale is probably that system administrators are still able to limit those clamps thanks to the cgroup interface. However, this causes pain in a system where both per-task and per-cgroup clamp values are expected to be under the control of core system components (as is the case for Android).

To fix this, let's require CAP_SYS_NICE to increase per-task clamp values. This allows unprivileged tasks to lower their requests, but not increase them, which is consistent with the existing behaviour for nice values.

Hmmm. I'm not in favour of this.

So uclamp is a performance and power management mechanism, it has no impact on fairness AFAICT, so it being a privileged operation doesn't make sense.

We had a thought about this in the past and we didn't think there's any harm if a task (app) wants to self manage. Yes a task could ask to run at max performance and waste power, but anyone can generate a busy loop and waste power too.

Quoted 8 lines

Quoted 4 lines

Now that doesn't mean your use case is not valid. I agree if there's a system wide framework that wants to explicitly manage performance and power of tasks via uclamp, then we can end up with 2 layers of controls overriding each others.

Right, that's the main issue. Also, the reality is that most of time the 'right' clamps are platform-dependent, so most userspace apps are simply not equipped to decide what their own clamps should be.

I'd argue this is true for both a framework or an app point of view. It depends on the application and how it would be used.

Quoted 4 lines

I can foresee for example and HTTP server wanting to use uclamp to guarantee a QoS target ie: X number of requests per second or a maximum of Y tail latency. The application can try to tune (calibrate) itself without having to have the whole system tuned or pumped on steroid.

Right, but the problem I see with this approach is that the app only understand its own performance, but is unable to decide what is best for the overall system health.

Anyway, it sounds like we agree that having _some_ way of limiting this would be useful, so we're all good.

Quoted 12 lines

Or a framework could manage this on behalf of the application. Both can use uclamp with a feedback loop to calibrate the perf requirement of the tasks to meet a given perf/power criteria.

If you want to do a static management, system framework would make more sense in this case, true.

Quoted 4 lines

Quoted 4 lines

Would it make more sense to have a procfs/sysfs flag that is disabled by default that allows sys-admin to enforce a privileged uclamp access?

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Something like

/proc/sys/kernel/sched_uclamp_privileged

Hmm, dunno, I'm not aware of anything else having a behaviour like that, so that feels a bit odd.

I think /proc/sys/kernel/perf_event_paranoid falls into this category.

Aha, so I'm guessing this was introduced as a sysfs knob rather than a CAP because it is a non-binary knob, but it's an interesting example.

Quoted 10 lines

Quoted 8 lines

Quoted 3 lines

I think both usage scenarios are valid and giving sys-admins the power to enforce a behavior makes more sense for me.

Yes, I wouldn't mind something like that in general. I originally wanted to suggest introducing a dedicated capability for uclamp, but that felt a bit overkill. Now if others think this should be the way to go I'm happy to go implement it.

Would be good to hear what others think for sure :)

Thinking about it a bit more, a more involved option would be to have this patch as is, but to also introduce a new RLIMIT_UCLAMP on top of it. The semantics could be:

- if the clamp requested by the non-privileged task is lower than its

    existing clamp, then allow;
  - otherwise, if the requested clamp is less than UCLAMP_RLIMIT, then
    allow;

- otherwise, deny,

And the same principle would apply to both uclamp.min and uclamp.max, and UCLAMP_RLIMIT would default to 0.

Thoughts?

Quoted 10 lines · Quentin Perret

Thinking about it a bit more, a more involved option would be to have this patch as is, but to also introduce a new RLIMIT_UCLAMP on top of it. The semantics could be:

- if the clamp requested by the non-privileged task is lower than its

    existing clamp, then allow;
  - otherwise, if the requested clamp is less than UCLAMP_RLIMIT, then
    allow;

- otherwise, deny,

Quoted 4 lines

And the same principle would apply to both uclamp.min and uclamp.max, and UCLAMP_RLIMIT would default to 0.

Thoughts?

That could work. But then I'd prefer your patch to go as-is. I don't think uclamp can do with this extra complexity in using it.

We basically want to specify we want to be paranoid about uclamp CAP or not. In my view that is simple and can't see why it would be a big deal to have a procfs entry to define the level of paranoia the system wants to impose. If it is a big deal though (would love to hear the arguments);

requiring apps that
want to self regulate to have CAP_SYS_NICE is better approach.

Quoted 14 lines · Qais Yousef

Quoted 13 lines · Quentin Perret

Thinking about it a bit more, a more involved option would be to have this patch as is, but to also introduce a new RLIMIT_UCLAMP on top of it. The semantics could be:

- if the clamp requested by the non-privileged task is lower than its

    existing clamp, then allow;
  - otherwise, if the requested clamp is less than UCLAMP_RLIMIT, then
    allow;

- otherwise, deny,

And the same principle would apply to both uclamp.min and uclamp.max, and UCLAMP_RLIMIT would default to 0.

Quoted 4 lines

Quoted 1 line

Thoughts?

That could work. But then I'd prefer your patch to go as-is. I don't think uclamp can do with this extra complexity in using it.

Sorry I'm not sure what you mean here?

Quoted 5 lines

We basically want to specify we want to be paranoid about uclamp CAP or not. In my view that is simple and can't see why it would be a big deal to have a procfs entry to define the level of paranoia the system wants to impose. If it is a big deal though (would love to hear the arguments);

Not saying it's a big deal, but I think there are a few arguments in favor of using rlimit instead of a sysfs knob. It allows for a much finer grain configuration -- constraints can be set per-task as well as system wide if needed, and it is the standard way of limiting resources that tasks can ask for.

Quoted 3 lines

requiring apps that
want to self regulate to have CAP_SYS_NICE is better approach.

Rlimit wouldn't require that though, which is also nice as CAP_SYS_NICE grants you a lot more power than just clamps ...

Quoted 15 lines · Quentin Perret

Quoted 14 lines · Qais Yousef

Quoted 13 lines · Quentin Perret

Thinking about it a bit more, a more involved option would be to have this patch as is, but to also introduce a new RLIMIT_UCLAMP on top of it. The semantics could be:

- if the clamp requested by the non-privileged task is lower than its

    existing clamp, then allow;
  - otherwise, if the requested clamp is less than UCLAMP_RLIMIT, then
    allow;

- otherwise, deny,

And the same principle would apply to both uclamp.min and uclamp.max, and UCLAMP_RLIMIT would default to 0.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Thoughts?

That could work. But then I'd prefer your patch to go as-is. I don't think uclamp can do with this extra complexity in using it.

Sorry I'm not sure what you mean here?

Hmm. I understood this as a new flag to sched_setattr() syscall first, but now I get it. You want to use getrlimit()/setrlimit()/prlimit() API to impose a restriction. My comment was in regard to this being a sys call extension, which it isn't. So please ignore it.

Quoted 6 lines

Quoted 5 lines

We basically want to specify we want to be paranoid about uclamp CAP or not. In my view that is simple and can't see why it would be a big deal to have a procfs entry to define the level of paranoia the system wants to impose. If it is a big deal though (would love to hear the arguments);

Quoted 5 lines

Not saying it's a big deal, but I think there are a few arguments in favor of using rlimit instead of a sysfs knob. It allows for a much finer grain configuration -- constraints can be set per-task as well as system wide if needed, and it is the standard way of limiting resources that tasks can ask for.

Is it system wide or per user?

Quoted 6 lines

Quoted 3 lines

requiring apps that
want to self regulate to have CAP_SYS_NICE is better approach.

Rlimit wouldn't require that though, which is also nice as CAP_SYS_NICE grants you a lot more power than just clamps ...

Now I better understand your suggestion. It seems a viable option I agree. I need to digest it more still though. The devil is in the details :)

Shouldn't the default be RLIM_INIFINITY? ie: no limit?

We will need to add two limit, RLIMIT_UCLAMP_MIN/MAX, right?

We have the following hierarchy now:

1. System Wide (/proc/sys/kerenl/sched_util_clamp_min/max)
2. Cgroup
3. Per-Task

In that order of priority where 1 limits/overrides 2 and 3. And 2 limits/overrides 3.

Where do you see the RLIMIT fit in this hierarchy? It should be between 2 and 3, right? Cgroup settings should still win even if the user/processes were limited?

If the framework decided a user can't request any boost at all (can't increase its uclamp_min above 0). IIUC then setting the hard limit of RLIMIT_UCLAMP_MIN to 0 would achieve that, right?

Since the framework and the task itself would go through the same sched_setattr() call, how would the framework circumvent this limit? IIUC it has to raise the RLIMIT_UCLAMP_MIN first then perform sched_setattr() to request the boost value, right? Would this overhead be acceptable? It looks considerable to me.

Also, Will prlimit() allow you to go outside what was set for the user via setrlimit()? Reading the man pages it seems to override, so that should be fine.

For 1 (System Wide) limits, sched_setattr() requests are accepted, but the effective uclamp is *capped by* the system wide limit.

Were you thinking RLIMIT_UCLAMP* will behave similarly?

If they do, we have
consistent behavior with how the current system wide limits work; but this will break your use case because tasks can change the requested uclamp value for a task, albeit the effective value will be limited.

	RLIMIT_UCLAMP_MIN=512
	p->uclamp[UCLAMP_min] = 800	// this request is allowed but
					// Effective UCLAMP_MIN = 512

If not, then

	RLIMIT_UCLAMP_MIN=no limit
	p->uclamp[UCLAMP_min] = 800	// task changed its uclamp_min to 800
	RLIMIT_UCLAMP_MIN=512		// limit was lowered for task/user

what will happen to p->uclamp[UCLAMP_MIN] in this case? Will it be lowered to match the new limit? And this will be inconsistent with the current system wide limits we already have.

Sorry too many questions. I was mainly thinking loudly. I need to spend more time to dig into the details of how RLIMITs are imposed to understand how this could be a good fit. I already see some friction points that needs more thinking.