Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting

[Patrick Bellasi]

On 13-Mar 09:19, Peter Zijlstra wrote:
> On Tue, Mar 12, 2019 at 03:50:43PM +0000, Patrick Bellasi wrote:
> > On 12-Mar 16:20, Peter Zijlstra wrote:
> > > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > > > +/* Integer ceil-rounded range for each bucket */
> 
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> > > > +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
> 
>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> simply do not match.

Right, that don't match when UCLAMP_BUCKETS is a divider of
SCHED_CAPACITY_SCALE, i.e. when we use 8 or 16 buckets.

> > > Uhm, should that not me ((x+y-1)/y), aka. DIV_ROUND_UP(x,y) ?
> > 
> > Well, there is certainly some rounding to be done...
> > 
> > > The above would give 4 for 9/3, which is clearly buggered.
> > 
> > .. still the math above should work fine within the boundaries we
> > define for UCLAMP_BUCKET_DELTA (5..20 groups) and considering that
> > SCHED_CAPACITY_SCALE will never be smaller then 1024.
> 
> That's a very poor reason to write utter nonsense :-)
> 
> > The above is designed to shrink the topmost bucket wrt all the others
> > but it will never be smaller than ~30%.
> 
> 30% sounds like a lot, esp. for this range.

Well, that 30% is really just ~16 utiliation units on a scale of 1024
when buckets have a size of 52.

Still, yes, we can argue that's big but that's also the same error
generated by DIV_ROUND_UP() when UCLAMP_BUCKETS is not 8 or 16.

> > Here are the start values computed for each bucket using the math
> > above and the computed shrinking percentage for the topmost bucket:
> 
> If you use a regular rounding, the error is _much_ smaller:
> 
> $ for ((x=5;x<21;x++)) ; do let d=(1024+x/2)/x; let s=(x-1)*d; let e=1024-s; let p=100*(d-e)/d; echo $x $d $s $e $p%; done
                                    ^^^^^^^^^^^^^
> 5 205 820 204 0%
> 6 171 855 169 1%
> 7 146 876 148 -1%
> 8 128 896 128 0%
> 9 114 912 112 1%
> 10 102 918 106 -3%
> 11 93 930 94 -1%
> 12 85 935 89 -4%
> 13 79 948 76 3%
> 14 73 949 75 -2%
> 15 68 952 72 -5%
> 16 64 960 64 0%
> 17 60 960 64 -6%
> 18 57 969 55 3%
> 19 54 972 52 3%
> 20 51 969 55 -7%
> 
> Funnily enough, we have a helper for that too: DIV_ROUND_CLOSEST().
                                                 ^^^^^^^^^^^^^^^^^^^^
This is different than DIV_ROUND_UP() and actually better across the
full range.

> Now, if we go further, the error will obviously increase because we run
> out of precision, but even there, regular rounding will be better than
> either floor or ceil.

I don't think we will have to cover other values in the further but I
agree that this "closest rounding" is definitively better.

Thanks for spotting it, will update in v8.

-- 
#include <best/regards.h>

Patrick Bellasi