[{"id":1767266,"web_url":"http://patchwork.ozlabs.org/comment/1767266/","msgid":"","list_archive_url":null,"date":"2017-09-12T17:37:35","subject":"Re: [Qemu-devel] [PULL for-2.10 05/15] throttle: Remove\n\tthrottle_fix_bucket() / throttle_unfix_bucket()","submitter":{"id":5111,"url":"http://patchwork.ozlabs.org/api/people/5111/","name":"Peter Maydell","email":"peter.maydell@linaro.org"},"content":"On 31 August 2017 at 09:22, Stefan Hajnoczi wrote:\n> From: Alberto Garcia \n>\n> The throttling code can change internally the value of bkt->max if it\n> hasn't been set by the user. The problem with this is that if we want\n> to retrieve the original value we have to undo this change first. This\n> is ugly and unnecessary: this patch removes the throttle_fix_bucket()\n> and throttle_unfix_bucket() functions completely and moves the logic\n> to throttle_compute_wait().\n>\n> Signed-off-by: Alberto Garcia \n> Reviewed-by: Manos Pitsidianakis \n> Message-id: 5b0b9e1ac6eb208d709eddc7b09e7669a523bff3.1503580370.git.berto@igalia.com\n> Signed-off-by: Stefan Hajnoczi \n> ---\n> util/throttle.c | 62 +++++++++++++++++++++------------------------------------\n> 1 file changed, 23 insertions(+), 39 deletions(-)\n>\n> diff --git a/util/throttle.c b/util/throttle.c\n> index bde56fe3de..4e80a7ea54 100644\n> --- a/util/throttle.c\n> +++ b/util/throttle.c\n> @@ -95,23 +95,36 @@ static int64_t throttle_do_compute_wait(double limit, double extra)\n> int64_t throttle_compute_wait(LeakyBucket *bkt)\n> {\n> double extra; /* the number of extra units blocking the io */\n> + double bucket_size; /* I/O before throttling to bkt->avg */\n> + double burst_bucket_size; /* Before throttling to bkt->max */\n>\n> if (!bkt->avg) {\n> return 0;\n> }\n>\n> - /* If the bucket is full then we have to wait */\n> - extra = bkt->level - bkt->max * bkt->burst_length;\n> + if (!bkt->max) {\n> + /* If bkt->max is 0 we still want to allow short bursts of I/O\n> + * from the guest, otherwise every other request will be throttled\n> + * and performance will suffer considerably. */\n> + bucket_size = bkt->avg / 10;\n> + burst_bucket_size = 0;\n> + } else {\n> + /* If we have a burst limit then we have to wait until all I/O\n> + * at burst rate has finished before throttling to bkt->avg */\n> + bucket_size = bkt->max * bkt->burst_length;\n> + burst_bucket_size = bkt->max / 10;\n> + }\n> +\n> + /* If the main bucket is full then we have to wait */\n> + extra = bkt->level - bucket_size;\n> if (extra > 0) {\n> return throttle_do_compute_wait(bkt->avg, extra);\n> }\n>\n> - /* If the bucket is not full yet we have to make sure that we\n> - * fulfill the goal of bkt->max units per second. */\n> + /* If the main bucket is not full yet we still have to check the\n> + * burst bucket in order to enforce the burst limit */\n> if (bkt->burst_length > 1) {\n> - /* We use 1/10 of the max value to smooth the throttling.\n> - * See throttle_fix_bucket() for more details. */\n> - extra = bkt->burst_level - bkt->max / 10;\n> + extra = bkt->burst_level - burst_bucket_size;\n> if (extra > 0) {\n> return throttle_do_compute_wait(bkt->max, extra);\n> }\n\nCoverity thinks there's a division-by-zero issue here: bkt->max could\nbe zero (we have a test for that up above), but we can here pass it\nto throttle_do_compute_wait(), which uses it as a divisor.\n\nSince this is all double arithmetic, the division isn't going\nto cause a crash, but the implicit cast of the resulting infinity\nto int64_t to return it is undefined behaviour.\n\nThis is CID 1381016.\n\nthanks\n-- PMM","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"FuS5Nhee\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xsBpv6wN6z9s82\n\tfor ;\n\tWed, 13 Sep 2017 03:38:31 +1000 (AEST)","from localhost ([::1]:37820 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drp8o-0003Qu-3l\n\tfor incoming@patchwork.ozlabs.org; Tue, 12 Sep 2017 13:38:30 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:55614)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drp8J-0003Pe-NF\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 13:38:00 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drp8H-00044K-Uy\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 13:37:59 -0400","from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]:45581)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1drp8H-00042E-NI\n\tfor qemu-devel@nongnu.org; Tue, 12 Sep 2017 13:37:57 -0400","by mail-wm0-x22c.google.com with SMTP id f199so529740wme.0\n\tfor ; Tue, 12 Sep 2017 10:37:57 -0700 (PDT)","by 10.223.139.215 with HTTP; Tue, 12 Sep 2017 10:37:35 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=8IwfpOm20DhCW2hOEbdBCpBG14pITS+x7yMs+NAks6Y=;\n\tb=FuS5Nheew0YAfUf9TRA1pKeSPgIS0CDCsx5jh6F6qAHh/31XbJ1p5yvg21oYlMEzh4\n\tMHnco1CqZ8B00+6SvyOeG+h28N1k5wdnHLVtts+yHYmColvF2xJxnKHTLY+FPQUhRivT\n\tqYUom6/cpEjItwv01xvif6CAlsswzJ8Wbpg9k=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=8IwfpOm20DhCW2hOEbdBCpBG14pITS+x7yMs+NAks6Y=;\n\tb=XYtKjk7QxLPOG835YxvaVQIFjxvSjqIqlsa2mBq0kXWA+y33+8ftvbBCDKoLimwsgC\n\tYdYBIMR6IhpJvE22k+v5k0gj88khmpz7r5C4Ca36CGlkg61fqX35qRCB8uBhmAQlkDmk\n\t3ps9W3jGSX6FZNGtSv0YusCmMY1eFeAzWzjsr53qElyO529M9I85F1Z7IwAtvPgd2OwB\n\t8ZUs7ORx6JsdZHYoTiWqM1ExMu3WCoufMNKzUd40Qx6xdnWBoJkzm26iNxodE05uHmks\n\t7vo3y+qxwH7se9jILhPRMgFrhlCf0K2WpOTQ32g5IEPJdTeho0Nbzy0wn/PotwrwgIYl\n\tIVHQ==","X-Gm-Message-State":"AHPjjUiIT+yBlXRKkt1HrUiw8I6mJpzuO5GQk4QvSyFtiaJ/5yWmELKs\n\trrk0ybE8uKYlLUuBCknLPeDZWiAoIbA/cZfI8caRNQ==","X-Google-Smtp-Source":"AOwi7QB1YP2aBheO4GSJ4lRBPwUu1nCok0E3bpnVrEMX3m4wcC/s725Qh1e5GTww78G1WvwMByrXa6mzNI5fDPvskwE=","X-Received":"by 10.28.23.5 with SMTP id 5mr273577wmx.62.1505237876543; Tue, 12\n\tSep 2017 10:37:56 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170831082210.8362-6-stefanha@redhat.com>","References":"<20170831082210.8362-1-stefanha@redhat.com>\n\t<20170831082210.8362-6-stefanha@redhat.com>","From":"Peter Maydell ","Date":"Tue, 12 Sep 2017 18:37:35 +0100","Message-ID":"","To":"Stefan Hajnoczi ","Content-Type":"text/plain; charset=\"UTF-8\"","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2a00:1450:400c:c09::22c","Subject":"Re: [Qemu-devel] [PULL for-2.10 05/15] throttle: Remove\n\tthrottle_fix_bucket() / throttle_unfix_bucket()","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Alberto Garcia ,\n\tQEMU Developers ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]