On 13-Mar 17:29, Suren Baghdasaryan wrote:

On Wed, Mar 13, 2019 at 9:16 AM Patrick Bellasi [off-list ref] wrote:

quoted

On 13-Mar 15:12, Peter Zijlstra wrote:

quoted

On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:

quoted

+static inline void uclamp_idle_reset(struct rq *rq, unsigned int clamp_id,
+                                unsigned int clamp_value)
+{
+   /* Reset max-clamp retention only on idle exit */
+   if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
+           return;
+
+   WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
+
+   /*
+    * This function is called for both UCLAMP_MIN (before) and UCLAMP_MAX
+    * (after). The idle flag is reset only the second time, when we know
+    * that UCLAMP_MIN has been already updated.

Why do we care? That is, what is this comment trying to tell us.

Right, the code is clear enough, I'll remove this comment.

It would be probably even clearer if rq->uclamp_flags &=
~UCLAMP_FLAG_IDLE is done from inside uclamp_rq_inc after
uclamp_rq_inc_id for both clamps is called.

Good point! I'll move it there to have something like:

---8<---
static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
{
	unsigned int clamp_id;

	if (unlikely(!p->sched_class->uclamp_enabled))
		return;

	for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
		uclamp_rq_inc_id(p, rq, clamp_id);

	/* Reset clamp holding when we have at least one RUNNABLE task */
	if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
		rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
---8<---

-- 
#include <best/regards.h>

Patrick Bellasi