From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VAISz-0001H5-67 for qemu-devel@nongnu.org; Fri, 16 Aug 2013 07:45:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VAISq-0005fI-N2 for qemu-devel@nongnu.org; Fri, 16 Aug 2013 07:45:17 -0400 Received: from mail-wg0-x232.google.com ([2a00:1450:400c:c00::232]:47507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VAISq-0005dt-GW for qemu-devel@nongnu.org; Fri, 16 Aug 2013 07:45:08 -0400 Received: by mail-wg0-f50.google.com with SMTP id m15so1466086wgh.17 for ; Fri, 16 Aug 2013 04:45:07 -0700 (PDT) Date: Fri, 16 Aug 2013 13:45:05 +0200 From: Stefan Hajnoczi Message-ID: <20130816114505.GA22193@stefanha-thinkpad.redhat.com> References: <1376326396-7676-1-git-send-email-benoit@irqsave.net> <1376326396-7676-2-git-send-email-benoit@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1376326396-7676-2-git-send-email-benoit@irqsave.net> Subject: Re: [Qemu-devel] [PATCH V5 1/5] throttle: Add a new throttling API implementing continuous leaky bucket. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Mon, Aug 12, 2013 at 06:53:12PM +0200, Benoît Canet wrote: > +#ifndef THROTTLING_H > +#define THROTTLING_H THROTTLE_H > + > +#include > +#include "qemu-common.h" > +#include "qemu/timer.h" > + > +#define NANOSECONDS_PER_SECOND 1000000000.0 > + > +#define BUCKETS_COUNT 6 > + > +typedef enum { > + THROTTLE_BPS_TOTAL = 0, > + THROTTLE_BPS_READ = 1, > + THROTTLE_BPS_WRITE = 2, > + THROTTLE_OPS_TOTAL = 3, > + THROTTLE_OPS_READ = 4, > + THROTTLE_OPS_WRITE = 5, > +} BucketType; > + > +typedef struct LeakyBucket { > + double ups; /* units per second */ > + double max; /* leaky bucket max in units */ > + double bucket; /* bucket in units */ These comments aren't very clear to me :). So I guess bps or iops would be in ups. Max would be the total budget or maximum burst. Bucket might be the current level. > +} LeakyBucket; > + > +/* The following structure is used to configure a ThrottleState > + * It contains a bit of state: the bucket field of the LeakyBucket structure. > + * However it allows to keep the code clean and the bucket field is reset to > + * zero at the right time. > + */ > +typedef struct ThrottleConfig { > + LeakyBucket buckets[6]; /* leaky buckets */ s/6/THROTTLE_TYPE_MAX/ > + uint64_t unit_size; /* size of an unit in bytes */ > + uint64_t op_size; /* size of an operation in units */ It's not clear yet why we need both unit_size *and* op_size. I thought you would have a single granularity field for accounting big requests as multiple iops. > +/* This function make a bucket leak > + * > + * @bkt: the bucket to make leak > + * @delta: the time delta delta is in nanoseconds. Probably best to call it delta_ns. > +/* destroy a timer */ > +static void throttle_timer_destroy(QEMUTimer **timer) > +{ > + assert(*timer != NULL); > + > + if (qemu_timer_pending(*timer)) { > + qemu_del_timer(*timer); > + } You can always call qemu_del_timer(), the timer doesn't need to be pending. > +/* fix bucket parameters */ > +static void throttle_fix_bucket(LeakyBucket *bkt) > +{ > + double min = bkt->ups / 10; > + /* zero bucket level */ > + bkt->bucket = 0; > + > + /* take care of not using cpu and also improve throttling precision */ > + if (bkt->ups && > + bkt->max < min) { > + bkt->max = min; > + } > +} This function seems like magic. What is really going on here? Why divide by 10 and when does this case happen? > + > +/* take care of canceling a timer */ > +static void throttle_cancel_timer(QEMUTimer *timer) > +{ > + assert(timer != NULL); > + if (!qemu_timer_pending(timer)) { > + return; > + } No need to check pending first.