Re: [Qemu-devel] rate limiting issues

[Stefan Hajnoczi]

On Tue, Feb 06, 2018 at 11:54:51AM +0100, Wolfgang Bumiller wrote:
> On Mon, Feb 05, 2018 at 03:31:40PM +0000, Stefan Hajnoczi wrote:
> > On Fri, Feb 02, 2018 at 12:10:22PM +0100, Wolfgang Bumiller wrote:
> (...)
> > > Explanation:
> > > The ratelimiting code in include/qemu/ratelimit.h currently uses slices 
> > > with
> > > quotas. Finishing up the quota for one slice means it'll wait for the end 
> > > of
> > > this _and_ the next slice before resetting the accounting and start over.
> > > If that first slice was exceeded by only a tiny bit, we effectively spend 
> > > every
> > > second slice waiting around. before starting over.
> (...)
> > > typedef struct {
> > >     int64_t last_time;
> > >     uint64_t speed;
> > >     int64_t allowed;
> > > } RateLimit;
> > > 
> > > static inline int64_t ratelimit_calculate_delay(RateLimit *limit, 
> > > uint64_t n)
> > > {
> (... wrong code)
> > > }
> > 
> > This does not implement a rate limit, at least not in the way that users
> > expect:
> > 
> > Imagine there is no activity for 24 hours.  We accumulate a huge number
> > of allowed units and can exceed the rate limit for some time.
> 
> Indeed, I forgot that blockjobs can be paused, which affects both
> versions.
> So we need slices, but do they need to be aligned? Changing the current
> code to not try align the waiting period with where a slice would start
> also seems to work better...
> 
> ---8<---
> From e50097e88a04eae0d1d40bed5fb940dc4baa835d Mon Sep 17 00:00:00 2001
> From: Wolfgang Bumiller <address@hidden>
> Date: Tue, 6 Feb 2018 11:34:34 +0100
> Subject: [PATCH] ratelimit: don't align wait time with slices
> 
> It is possible for rate limited writes to keep overshooting a slice's
> quota by a tiny amount causing the slice-aligned waiting period to
> effectively halve the rate.
> 
> Signed-off-by: Wolfgang Bumiller <address@hidden>
> ---
>  include/qemu/ratelimit.h | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h
> index 8da1232574..01a5d535ec 100644
> --- a/include/qemu/ratelimit.h
> +++ b/include/qemu/ratelimit.h
> @@ -35,7 +35,7 @@ typedef struct {
>  static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
>  {
>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    uint64_t delay_slices;
> +    double delay_slices;
>  
>      assert(limit->slice_quota && limit->slice_ns);
>  
> @@ -54,12 +54,11 @@ static inline int64_t ratelimit_calculate_delay(RateLimit 
> *limit, uint64_t n)
>          return 0;
>      }
>  
> -    /* Quota exceeded. Calculate the next time slice we may start
> -     * sending data again. */
> -    delay_slices = (limit->dispatched + limit->slice_quota - 1) /
> -        limit->slice_quota;
> +    /* Quota exceeded. Wait based on the excess amount and then start a new
> +     * slice. */
> +    delay_slices = (double)limit->dispatched / limit->slice_quota;
>      limit->slice_end_time = limit->slice_start_time +
> -        delay_slices * limit->slice_ns;
> +        (uint64_t)(delay_slices * limit->slice_ns);
>      return limit->slice_end_time - now;
>  }

Looks good, thanks!  Please send the patch as a top-level email thread
so it can be merged:
https://wiki.qemu.org/Contribute/SubmitAPatch

Stefan

signature.asc
Description: PGP signature