Re: [PATCH V2 04/16] block, bfq: modify the peak-rate estimator

[Bart Van Assche]

On Fri, 2017-03-31 at 14:47 +0200, Paolo Valente wrote:
> -static bool bfq_update_peak_rate(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> -                                bool compensate)
> +static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> +                                bool compensate, enum bfqq_expiration reason,
> +                                unsigned long *delta_ms)
>  {
> -       u64 bw, usecs, expected, timeout;
> -       ktime_t delta;
> -       int update = 0;
> +       ktime_t delta_ktime;
> +       u32 delta_usecs;
> +       bool slow = BFQQ_SEEKY(bfqq); /* if delta too short, use seekyness */
>  
> -       if (!bfq_bfqq_sync(bfqq) || bfq_bfqq_budget_new(bfqq))
> +       if (!bfq_bfqq_sync(bfqq))
>                 return false;
>  
>         if (compensate)
> -               delta = bfqd->last_idling_start;
> +               delta_ktime = bfqd->last_idling_start;
>         else
> -               delta = ktime_get();
> -       delta = ktime_sub(delta, bfqd->last_budget_start);
> -       usecs = ktime_to_us(delta);
> -
> -       /* Don't trust short/unrealistic values. */
> -       if (usecs < 100 || usecs >= LONG_MAX)
> -               return false;
> -
> -       /*
> -        * Calculate the bandwidth for the last slice.  We use a 64 bit
> -        * value to store the peak rate, in sectors per usec in fixed
> -        * point math.  We do so to have enough precision in the estimate
> -        * and to avoid overflows.
> -        */
> -       bw = (u64)bfqq->entity.service << BFQ_RATE_SHIFT;
> -       do_div(bw, (unsigned long)usecs);
> +               delta_ktime = ktime_get();
> +       delta_ktime = ktime_sub(delta_ktime, bfqd->last_budget_start);
> +       delta_usecs = ktime_to_us(delta_ktime);
> +

This patch changes the type of the variable in which the result of ktime_to_us()
is stored from u64 into u32 and next compares that result with LONG_MAX. Since
ktime_to_us() returns a signed 64-bit number, are you sure you want to store that
result in a 32-bit variable? If ktime_to_us() would e.g. return 0xffffffff00000100
or 0x100000100 then the assignment will truncate these numbers to 0x100.

Bart.