On 31 August 2017 at 09:22, Stefan Hajnoczi <stefa...@redhat.com> wrote: > From: Alberto Garcia <be...@igalia.com> > > The throttling code can change internally the value of bkt->max if it > hasn't been set by the user. The problem with this is that if we want > to retrieve the original value we have to undo this change first. This > is ugly and unnecessary: this patch removes the throttle_fix_bucket() > and throttle_unfix_bucket() functions completely and moves the logic > to throttle_compute_wait(). > > Signed-off-by: Alberto Garcia <be...@igalia.com> > Reviewed-by: Manos Pitsidianakis <el13...@mail.ntua.gr> > Message-id: > 5b0b9e1ac6eb208d709eddc7b09e7669a523bff3.1503580370.git.be...@igalia.com > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > util/throttle.c | 62 > +++++++++++++++++++++------------------------------------ > 1 file changed, 23 insertions(+), 39 deletions(-) > > diff --git a/util/throttle.c b/util/throttle.c > index bde56fe3de..4e80a7ea54 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -95,23 +95,36 @@ static int64_t throttle_do_compute_wait(double limit, > double extra) > int64_t throttle_compute_wait(LeakyBucket *bkt) > { > double extra; /* the number of extra units blocking the io */ > + double bucket_size; /* I/O before throttling to bkt->avg */ > + double burst_bucket_size; /* Before throttling to bkt->max */ > > if (!bkt->avg) { > return 0; > } > > - /* If the bucket is full then we have to wait */ > - extra = bkt->level - bkt->max * bkt->burst_length; > + if (!bkt->max) { > + /* If bkt->max is 0 we still want to allow short bursts of I/O > + * from the guest, otherwise every other request will be throttled > + * and performance will suffer considerably. */ > + bucket_size = bkt->avg / 10; > + burst_bucket_size = 0; > + } else { > + /* If we have a burst limit then we have to wait until all I/O > + * at burst rate has finished before throttling to bkt->avg */ > + bucket_size = bkt->max * bkt->burst_length; > + burst_bucket_size = bkt->max / 10; > + } > + > + /* If the main bucket is full then we have to wait */ > + extra = bkt->level - bucket_size; > if (extra > 0) { > return throttle_do_compute_wait(bkt->avg, extra); > } > > - /* If the bucket is not full yet we have to make sure that we > - * fulfill the goal of bkt->max units per second. */ > + /* If the main bucket is not full yet we still have to check the > + * burst bucket in order to enforce the burst limit */ > if (bkt->burst_length > 1) { > - /* We use 1/10 of the max value to smooth the throttling. > - * See throttle_fix_bucket() for more details. */ > - extra = bkt->burst_level - bkt->max / 10; > + extra = bkt->burst_level - burst_bucket_size; > if (extra > 0) { > return throttle_do_compute_wait(bkt->max, extra); > }
Coverity thinks there's a division-by-zero issue here: bkt->max could be zero (we have a test for that up above), but we can here pass it to throttle_do_compute_wait(), which uses it as a divisor. Since this is all double arithmetic, the division isn't going to cause a crash, but the implicit cast of the resulting infinity to int64_t to return it is undefined behaviour. This is CID 1381016. thanks -- PMM