Re: [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns()

[Luc Michel <luc.michel@greensocs.com>]

On 12/15/20 4:09 PM, Peter Maydell wrote:
> The clock_get_ns() API claims to return the period of a clock in
> nanoseconds. Unfortunately since it returns an integer and a
> clock's period is represented in units of 2^-32 nanoseconds,
> the result is often an approximation, and calculating a clock
> expiry deadline by multiplying clock_get_ns() by a number-of-ticks
> is unacceptably inaccurate.
> 
> Introduce a new API clock_ticks_to_ns() which returns the number
> of nanoseconds it takes the clock to make a given number of ticks.
> This function can do the complete calculation internally and
> will thus give a more accurate result.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> The 64x64->128 multiply is a bit painful for 32-bit and I
> guess in theory since we know we only want bits [95:32]
> of the result we could special-case it, but TBH I don't
> think 32-bit hosts merit much optimization effort these days.
> 
> Changes in v2: saturate the result to INT64_MAX.
> ---
>   docs/devel/clocks.rst | 29 +++++++++++++++++++++++++++++
>   include/hw/clock.h    | 41 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 70 insertions(+)
> 
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index e5da28e2111..c2e70e64db1 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -258,6 +258,35 @@ Here is an example:
>                           clock_get_ns(dev->my_clk_input));
>       }
>   
> +Calculating expiry deadlines
> +----------------------------
> +
> +A commonly required operation for a clock is to calculate how long
> +it will take for the clock to tick N times; this can then be used
> +to set a timer expiry deadline. Use the function ``clock_ticks_to_ns()``,
> +which takes an unsigned 64-bit count of ticks and returns the length
> +of time in nanoseconds required for the clock to tick that many times.
> +
> +It is important not to try to calculate expiry deadlines using a
> +shortcut like multiplying a "period of clock in nanoseconds" value
> +by the tick count, because clocks can have periods which are not a
> +whole number of nanoseconds, and the accumulated error in the
> +multiplication can be significant.
> +
> +For a clock with a very long period and a large number of ticks,
> +the result of this function could in theory be too large to fit in
> +a 64-bit value. To avoid overflow in this case, ``clock_ticks_to_ns()``
> +saturates the result to INT64_MAX (because this is the largest valid
> +input to the QEMUTimer APIs). Since INT64_MAX nanoseconds is almost
> +300 years, anything with an expiry later than that is in the "will
> +never happen" category. Callers of ``clock_ticks_to_ns()`` should
> +therefore generally not special-case the possibility of a saturated
> +result but just allow the timer to be set to that far-future value.
> +(If you are performing further calculations on the returned value
> +rather than simply passing it to a QEMUTimer function like
> +``timer_mod_ns()`` then you should be careful to avoid overflow
> +in those calculations, of course.)
> +
>   Changing a clock period
>   -----------------------
>   
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index 81bcf3e505a..b5fff6ded83 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -16,6 +16,8 @@
>   
>   #include "qom/object.h"
>   #include "qemu/queue.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/bitops.h"
>   
>   #define TYPE_CLOCK "clock"
>   OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
> @@ -218,6 +220,45 @@ static inline unsigned clock_get_ns(Clock *clk)
>       return CLOCK_PERIOD_TO_NS(clock_get(clk));
>   }
>   
> +/**
> + * clock_ticks_to_ns:
> + * @clk: the clock to query
> + * @ticks: number of ticks
> + *
> + * Returns the length of time in nanoseconds for this clock
> + * to tick @ticks times. Because a clock can have a period
> + * which is not a whole number of nanoseconds, it is important
> + * to use this function when calculating things like timer
> + * expiry deadlines, rather than attempting to obtain a "period
> + * in nanoseconds" value and then multiplying that by a number
> + * of ticks.
> + *
> + * The result could in theory be too large to fit in a 64-bit
> + * value if the number of ticks and the clock period are both
> + * large; to avoid overflow the result will be saturated to INT64_MAX
> + * (because this is the largest valid input to the QEMUTimer APIs).
> + * Since INT64_MAX nanoseconds is almost 300 years, anything with
> + * an expiry later than that is in the "will never happen" category
> + * and callers can reasonably not special-case the saturated result.
> + */
> +static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
> +{
> +    uint64_t ns_low, ns_high;
> +
> +    /*
> +     * clk->period is the period in units of 2^-32 ns, so
> +     * (clk->period * ticks) is the required length of time in those
> +     * units, and we can convert to nanoseconds by multiplying by
> +     * 2^32, which is the same as shifting the 128-bit multiplication
> +     * result right by 32.
> +     */
> +    mulu64(&ns_low, &ns_high, clk->period, ticks);
> +    if (ns_high & MAKE_64BIT_MASK(31, 33)) {
> +        return INT64_MAX;
> +    }
> +    return ns_low >> 32 | ns_high << 32;
> +}
> +
>   /**
>    * clock_is_enabled:
>    * @clk: a clock
>