[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL for-2.10 05/15] throttle: Remove throttle_fix_buc
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PULL for-2.10 05/15] throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket() |
Date: |
Tue, 12 Sep 2017 18:37:35 +0100 |
On 31 August 2017 at 09:22, Stefan Hajnoczi <address@hidden> wrote:
> From: Alberto Garcia <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Manos Pitsidianakis <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PULL for-2.10 05/15] throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket(),
Peter Maydell <=