`On 06-Apr 16:51, Suren Baghdasaryan wrote:> On Tue, Apr 2, 2019 at 3:42 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:[...]> > + * 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 ?          \> > > +                       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 youpropose should work for all n>=2.> > > +               )                               \> > +       ) :                                     \> > +       __bits_per(n)                           \> > +)> >  #endif /* _LINUX_LOG2_H */[...]> > +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 Peterrequest:   Message-ID: <20190314111849.gx6bl6myfjtaan7r@e110439-lin>This functions was left here to support a the intialization code ininit_uclamp() but... I notice know I'm doing the initialization in adifferent way thus, I'll move it into the following patch.> > +{> > +       return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);> > +}> > +[...]> > +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 neverreset it to its nominal value. In this patch specifically, the valueis never changed since we moved the "local max tracking" bits into adedicated patch.> > +               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));> > +}[...]> > +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 nextpatch introducing "local max tracking".> > +> > +       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`