Re: [Qemu-block] [PATCH 1/1] Improve block job rate limiting for small bandwidth values

[Max Reitz]

On 04.07.2016 16:30, Sascha Silbe wrote:
> Dear Max,
> 
> Max Reitz <address@hidden> writes:
> 
>> On 28.06.2016 17:28, Sascha Silbe wrote:
> [block/mirror.c]
>>> @@ -416,7 +416,9 @@ static uint64_t coroutine_fn 
>>> mirror_iteration(MirrorBlockJob *s)
>>>          assert(io_sectors);
>>>          sector_num += io_sectors;
>>>          nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk);
>>> -        delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors);
>>> +        if (s->common.speed) {
>>> +            delay_ns = ratelimit_calculate_delay(&s->limit, io_sectors);
>>> +        }
>>
>> Hmm... Was it a bug that ratelimit_calculate_delay() was called
>> unconditionally before?
> 
> One could argue either way. It happened to work because
> ratelimit_calculate_delay() only delayed the _second_ time (within one
> time slice) the quota was exceeded. With zero duration time slices,
> there never was a second time.
> 
> With the new implementation we would divide by zero when slice_quota is
> 0, so we need to guard against that. Most callers already did, only
> mirror_iteration() needed to be adjusted.
> 
> 
> [...]
> [include/qemu/ratelimit.h]
>>>  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;
>>>  
>>> -    if (limit->next_slice_time < now) {
>>> -        limit->next_slice_time = now + limit->slice_ns;
>>> +    assert(limit->slice_quota && limit->slice_ns);
>>> +
>>> +    if (limit->slice_end_time < now) {
>>> +        /* Previous, possibly extended, time slice finished; reset the
>>> +         * accounting. */
>>> +        limit->slice_start_time = now;
>>> +        limit->slice_end_time = now + limit->slice_ns;
>>>          limit->dispatched = 0;
>>>      }
>>> -    if (limit->dispatched == 0 || limit->dispatched + n <= 
>>> limit->slice_quota) {
>>> -        limit->dispatched += n;
>>> +
>>> +    limit->dispatched += n;
>>> +    if (limit->dispatched < limit->slice_quota) {
>>
>> Nitpick: This should probably stay <=.
> 
> This is a subtle edge case. Previously, when limit->dispatched ==
> limit->slice_quota, we returned 0 so that the _current_ request (which
> is still within quota) wouldn't be delayed. Now, we return a delay so
> that the _next_ request (which would be over quota) will be delayed.

Hm, but that depends on the size of the next request. Of course, if we
get limit->dispatched == limit->slice_quota we know for sure that we
need to delay the next request. But if we get limit->dispatched ==
limit->slice_quota - 1... Then we probably also have to delay it, but we
don't know for sure.

So I think it would be better to have small but consistent systematic
error here, i.e. that we will not delay the last request even though we
should. Or you could insert a delay after the last request in all block
jobs, too.

Or did I fail to understand the issue? I'm not sure.

> [...]
>>> +    /* Quota exceeded. Calculate the next time slice we may start
>>> +     * sending data again. */
>>> +    delay_slices = (limit->dispatched + limit->slice_quota - 1) /
>>> +        limit->slice_quota;
>>> +    limit->slice_end_time = limit->slice_start_time +
>>> +        delay_slices * limit->slice_ns;
>>
>> I think it would make sense to make this a floating point calculation.
> 
> Then we'd have fully variable length time slices, instead of just
> "occupying" multiple fixed-length time slices with a single
> request. Maybe that would be even better, or maybe we'd cause other
> interesting things to happen (think interactions with the scheduler).

:-)

>                                                                       As
> this code will hopefully disappear during the 2.8 time line anyway, I'd
> prefer to go with the lowest risk option that is enough to fix the race
> conditions encountered by the test suite.

OK with me.

Max

>> If you don't agree, though:
>>
>> Reviewed-by: Max Reitz <address@hidden>
> 
> Thanks for the review!
> 
> Sascha
>

signature.asc
Description: OpenPGP digital signature