Messages in this thread | | | Date | Thu, 16 Aug 2018 18:20:17 +0100 | From | Patrick Bellasi <> | Subject | Re: [PATCH v3 07/14] sched/core: uclamp: enforce last task UCLAMP_MAX |
| |
On 06-Aug 17:39, Patrick Bellasi wrote: > When a util_max clamped task sleeps, its clamp constraints are removed > from the CPU. However, the blocked utilization on that CPU can still be > higher than the max clamp value enforced while that task was running. > This max clamp removal when a CPU is going to be idle could thus allow > unwanted CPU frequency increases, right while the task is not running. > > This can happen, for example, where there is another (smaller) task > running on a different CPU of the same frequency domain. > In this case, when we aggregate the utilization of all the CPUs in a > shared frequency domain, schedutil can still see the full non clamped > blocked utilization of all the CPUs and thus eventually increase the > frequency. > > Let's fix this by using: > > uclamp_cpu_put_id(UCLAMP_MAX) > uclamp_cpu_update(last_clamp_value) > > to detect when a CPU has no more RUNNABLE clamped tasks and to flag this > condition. Thus, while a CPU is idle, we can still enforce the last used > clamp value for it. > > To the contrary, we do not track any UCLAMP_MIN since, while a CPU is > idle, we don't want to enforce any minimum frequency > Indeed, we rely just on blocked load decay to smoothly reduce the > frequency.
[...]
> @@ -906,7 +906,8 @@ uclamp_group_find(int clamp_id, unsigned int clamp_value) > * For the specified clamp index, this method computes the new CPU utilization > * clamp to use until the next change on the set of RUNNABLE tasks on that CPU. > */ > -static inline void uclamp_cpu_update(struct rq *rq, int clamp_id) > +static inline void uclamp_cpu_update(struct rq *rq, int clamp_id, > + unsigned int last_clamp_value) > { > struct uclamp_group *uc_grp = &rq->uclamp.group[clamp_id][0]; > int max_value = UCLAMP_NOT_VALID; > @@ -924,6 +925,19 @@ static inline void uclamp_cpu_update(struct rq *rq, int clamp_id) > if (max_value >= SCHED_CAPACITY_SCALE) > break; > } > + > + /* > + * Just for the UCLAMP_MAX value, in case there are no RUNNABLE > + * task, we keep the CPU clamped to the last task's clamp value. > + * This avoids frequency spikes to MAX when one CPU, with an high > + * blocked utilization, sleeps and another CPU, in the same frequency > + * domain, do not see anymore the clamp on the first CPU. > + */ > + if (clamp_id == UCLAMP_MAX && max_value == UCLAMP_NOT_VALID) { > + rq->uclamp.flags |= UCLAMP_FLAG_IDLE; > + max_value = last_clamp_value; > + } > + > rq->uclamp.value[clamp_id] = max_value; > } > > @@ -953,13 +967,26 @@ static inline void uclamp_cpu_get_id(struct task_struct *p, > uc_grp = &rq->uclamp.group[clamp_id][0]; > uc_grp[group_id].tasks += 1; > > + /* Force clamp update on idle exit */ > + uc_cpu = &rq->uclamp; > + clamp_value = p->uclamp[clamp_id].value; > + if (unlikely(uc_cpu->flags & UCLAMP_FLAG_IDLE)) { > + /* > + * This function is called for both UCLAMP_MIN (before) and > + * UCLAMP_MAX (after). Let's reset the flag only the second > + * once we know that UCLAMP_MIN has been already updated. > + */ > + if (clamp_id == UCLAMP_MAX) > + uc_cpu->flags &= ~UCLAMP_FLAG_IDLE; > + uc_cpu->value[clamp_id] = clamp_value; > + return; > + }
Just notice that the code block above is not reached when we enqueue a task without a valid clamp group, i.e. an un-clamped task, which is the default for all tasks.
The fix should be as simple as moving this block at the beginning of uclamp_cpu_update() so that we always un-conditionally release the clamp holding as soon as we enqueue the first task after a CPU wakeup, i.e. when the UCLAMP_FLAG_IDLE flag is set.
Will fix this on v4.
-- #include <best/regards.h>
Patrick Bellasi
| |