On 06-Apr 16:51, Suren Baghdasaryan wrote:

On Tue, Apr 2, 2019 at 3:42 AM Patrick Bellasi [off-list ref] wrote:

[...]

quoted

+ * The first few values calculated by this routine:
+ * bf(0) = 1
+ * bf(1) = 1
+ * bf(2) = 2
+ * bf(3) = 2
+ * bf(4) = 3
+ * ... and so on.
+ */
+#define bits_per(n)                            \
+(                                              \
+       __builtin_constant_p(n) ? (             \
+               ((n) == 0 || (n) == 1) ? 1 : (  \
+               ((n) & (n - 1)) == 0 ?          \

missing braces around 'n'
-               ((n) & (n - 1)) == 0 ?          \
+               ((n) & ((n) - 1)) == 0 ?          \

quoted

+                       ilog2((n) - 1) + 2 :    \
+                       ilog2((n) - 1) + 1      \

Isn't this "((n) & ((n) - 1)) == 0 ?  ilog2((n) - 1) + 2 : ilog2((n) -
1) + 1" expression equivalent to a simple "ilog2(n) + 1"?

Right, since we already have n=0 and n=1 as special cases, what you
propose should work for all n>=2.

quoted

+               )                               \
+       ) :                                     \
+       __bits_per(n)                           \
+)
 #endif /* _LINUX_LOG2_H */

[...]

quoted

+static inline unsigned int uclamp_bucket_base_value(unsigned int clamp_value)

Where are you using uclamp_bucket_base_value()? I would expect its
usage somewhere inside uclamp_rq_dec_id() when the last task in the
bucket is dequeued but I don't see it...

This behavior is not move into a dedicated patch, as per Peter
request:

   Message-ID: <20190314111849.gx6bl6myfjtaan7r@e110439-lin>

This functions was left here to support a the intialization code in
init_uclamp() but... I notice know I'm doing the initialization in a
different way thus, I'll move it into the following patch.

quoted

+{
+       return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
+}
+

[...]

quoted

+static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
+                                   unsigned int clamp_id)
+{
+       struct uclamp_rq *uc_rq = &rq->uclamp[clamp_id];
+       struct uclamp_se *uc_se = &p->uclamp[clamp_id];
+       struct uclamp_bucket *bucket;
+       unsigned int rq_clamp;
+
+       bucket = &uc_rq->bucket[uc_se->bucket_id];
+       SCHED_WARN_ON(!bucket->tasks);
+       if (likely(bucket->tasks))
+               bucket->tasks--;
+
+       if (likely(bucket->tasks))

Shouldn't you adjust bucket->value if the remaining tasks in the
bucket have a lower clamp value than the task that was just removed?

No, this is never done. As long as a bucket is not empty/idle we never
reset it to its nominal value. In this patch specifically, the value
is never changed since we moved the "local max tracking" bits into a
dedicated patch.

quoted

+               return;
+
+       rq_clamp = READ_ONCE(uc_rq->value);
+       /*
+        * Defensive programming: this should never happen. If it happens,
+        * e.g. due to future modification, warn and fixup the expected value.
+        */
+       SCHED_WARN_ON(bucket->value > rq_clamp);
+       if (bucket->value >= rq_clamp)
+               WRITE_ONCE(uc_rq->value, uclamp_rq_max_value(rq, clamp_id));
+}

[...]

quoted

+static void __init init_uclamp(void)
+{
+       unsigned int clamp_id;
+       int cpu;
+
+       for_each_possible_cpu(cpu) {
+               struct uclamp_bucket *bucket;
+               struct uclamp_rq *uc_rq;
+               unsigned int bucket_id;
+
+               memset(&cpu_rq(cpu)->uclamp, 0, sizeof(struct uclamp_rq));
+
+               for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) {
+                       uc_rq = &cpu_rq(cpu)->uclamp[clamp_id];
+
+                       bucket_id = 1;
+                       while (bucket_id < UCLAMP_BUCKETS) {
+                               bucket = &uc_rq->bucket[bucket_id];
+                               bucket->value = bucket_id * UCLAMP_BUCKET_DELTA;
+                               ++bucket_id;
+                       }
+               }
+       }

All the initialization code above is not more required after the next
patch introducing "local max tracking".

quoted

+
+       for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) {
+               struct uclamp_se *uc_se = &init_task.uclamp[clamp_id];
+
+               uc_se->value = uclamp_none(clamp_id);
+               uc_se->bucket_id = uclamp_bucket_id(uc_se->value);
+       }
+}

[...]

-- 
#include <best/regards.h>

Patrick Bellasi