* [PATCH v2 0/4] clock: Get rid of clock_get_ns()
@ 2020-12-15 15:09 Peter Maydell
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Peter Maydell @ 2020-12-15 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang,
Philippe Mathieu-Daudé
This patchseries makes some changes to the clock API:
* Remove clock_get_ns()
* Add clock_ticks_to_ns() to return number of nanoseconds
it will take the clock to tick N times
* clock_display_freq() to return prettily-formatted string
for showing humans the approximate clock frequency
This is based on discussions we had about these APIs a little while
back. The core driver here is that the clock objects internally
store the period in units of 2^-32 ns, so both clock_get_ns() and
clock_get_hz() are inherently returning a rounded-off result, which
can be badly inaccurate for fast clocks or if you want to multiply it
by a large tick count.
Ideally I'd like to get rid of clock_get_hz() as well, but
that looks trickier than handling clock_get_ns().
Patch 4 borrows a lot of the concept from one of Philippe's that he
sent out previously.
NB: tested with 'make check' and 'make check-acceptance' only.
Changes v1->v2:
* In patch 1, saturate return value to INT64_MAX (and update
docs to explain this and why)
Patches 2-4 have been reviewed, only patch 1 needs review.
thanks
-- PMM
Peter Maydell (4):
clock: Introduce clock_ticks_to_ns()
target/mips: Don't use clock_get_ns() in clock period calculation
clock: Remove clock_get_ns()
clock: Define and use new clock_display_freq()
docs/devel/clocks.rst | 51 ++++++++++++++++++++++++++++++++++++----
include/hw/clock.h | 53 +++++++++++++++++++++++++++++++++++++++---
hw/core/clock.c | 6 +++++
softmmu/qdev-monitor.c | 6 ++---
target/mips/cpu.c | 4 ++--
5 files changed, 108 insertions(+), 12 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns()
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
@ 2020-12-15 15:09 ` Peter Maydell
2020-12-15 15:21 ` Richard Henderson
2020-12-15 19:48 ` Luc Michel
2020-12-15 15:09 ` [PATCH v2 2/4] target/mips: Don't use clock_get_ns() in clock period calculation Peter Maydell
` (4 subsequent siblings)
5 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2020-12-15 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang,
Philippe Mathieu-Daudé
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>
---
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
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] target/mips: Don't use clock_get_ns() in clock period calculation
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
@ 2020-12-15 15:09 ` Peter Maydell
2020-12-15 15:09 ` [PATCH v2 3/4] clock: Remove clock_get_ns() Peter Maydell
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2020-12-15 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang,
Philippe Mathieu-Daudé
Currently the MIPS code uses the old clock_get_ns() API to
calculate a time length in nanoseconds:
cpu->cp0_count_rate * clock_get_ns(MIPS_CPU(cpu)->clock)
This relies on the clock having a period which is an exact number
of nanoseconds.
Switch to the new clock_ticks_to_ns() function, which does the
multiplication internally at a higher precision.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Luc Michel <luc@lmichel.fr>
---
target/mips/cpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index aadc6f8e74d..eea14f3b52f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -380,8 +380,8 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
{
CPUMIPSState *env = &cpu->env;
- env->cp0_count_ns = cpu->cp0_count_rate
- * clock_get_ns(MIPS_CPU(cpu)->clock);
+ env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
+ cpu->cp0_count_rate);
assert(env->cp0_count_ns);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] clock: Remove clock_get_ns()
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
2020-12-15 15:09 ` [PATCH v2 2/4] target/mips: Don't use clock_get_ns() in clock period calculation Peter Maydell
@ 2020-12-15 15:09 ` Peter Maydell
2020-12-15 15:09 ` [PATCH v2 4/4] clock: Define and use new clock_display_freq() Peter Maydell
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2020-12-15 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang,
Philippe Mathieu-Daudé
Remove the now-unused clock_get_ns() API and the CLOCK_PERIOD_TO_NS()
macro that only it was using.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Luc Michel <luc@lmichel.fr>
---
docs/devel/clocks.rst | 17 +++++++++++++----
include/hw/clock.h | 6 ------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index c2e70e64db1..2d317ff32f2 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -238,8 +238,17 @@ object during device instance init. For example:
Fetching clock frequency/period
-------------------------------
-To get the current state of a clock, use the functions ``clock_get()``,
-``clock_get_ns()`` or ``clock_get_hz()``.
+To get the current state of a clock, use the functions ``clock_get()``
+or ``clock_get_hz()``.
+
+``clock_get()`` returns the period of the clock in its fully precise
+internal representation, as an unsigned 64-bit integer in units of
+2^-32 nanoseconds. (For many purposes ``clock_ticks_to_ns()`` will
+be more convenient; see the section below on expiry deadlines.)
+
+``clock_get_hz()`` returns the frequency of the clock, rounded to the
+next lowest integer. This implies some inaccuracy due to the rounding,
+so be cautious about using it in calculations.
It is also possible to register a callback on clock frequency changes.
Here is an example:
@@ -254,8 +263,8 @@ Here is an example:
*/
/* do something with the new period */
- fprintf(stdout, "device new period is %" PRIu64 "ns\n",
- clock_get_ns(dev->my_clk_input));
+ fprintf(stdout, "device new period is %" PRIu64 "* 2^-32 ns\n",
+ clock_get(dev->my_clk_input));
}
Calculating expiry deadlines
diff --git a/include/hw/clock.h b/include/hw/clock.h
index b5fff6ded83..852c636961e 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -40,7 +40,6 @@ typedef void ClockCallback(void *opaque);
* macro helpers to convert to hertz / nanosecond
*/
#define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
-#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
#define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
#define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_PERIOD_1SEC / (per) : 0u)
@@ -215,11 +214,6 @@ static inline unsigned clock_get_hz(Clock *clk)
return CLOCK_PERIOD_TO_HZ(clock_get(clk));
}
-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
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] clock: Define and use new clock_display_freq()
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
` (2 preceding siblings ...)
2020-12-15 15:09 ` [PATCH v2 3/4] clock: Remove clock_get_ns() Peter Maydell
@ 2020-12-15 15:09 ` Peter Maydell
2020-12-15 15:29 ` [PATCH v2 0/4] clock: Get rid of clock_get_ns() Philippe Mathieu-Daudé
2021-01-01 20:35 ` Philippe Mathieu-Daudé
5 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2020-12-15 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang,
Philippe Mathieu-Daudé
It's common to want to print a human-readable indication of a clock's
frequency. Provide a utility function in the clock API to return a
string which is a displayable representation of the frequency,
and use it in qdev-monitor.c.
Before:
(qemu) info qtree
[...]
dev: xilinx,zynq_slcr, id ""
clock-in "ps_clk" freq_hz=3.333333e+07
mmio 00000000f8000000/0000000000001000
After:
dev: xilinx,zynq_slcr, id ""
clock-in "ps_clk" freq_hz=33.3 MHz
mmio 00000000f8000000/0000000000001000
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Luc Michel <luc@lmichel.fr>
---
This is based on Philippe's patch
"qdev-monitor: Display frequencies scaled to SI unit"
but I have abstracted out the "prettified string" into the clock API.
---
docs/devel/clocks.rst | 5 +++++
include/hw/clock.h | 12 ++++++++++++
hw/core/clock.c | 6 ++++++
softmmu/qdev-monitor.c | 6 +++---
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index 2d317ff32f2..2548d842322 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -267,6 +267,11 @@ Here is an example:
clock_get(dev->my_clk_input));
}
+If you are only interested in the frequency for displaying it to
+humans (for instance in debugging), use ``clock_display_freq()``,
+which returns a prettified string-representation, e.g. "33.3 MHz".
+The caller must free the string with g_free() after use.
+
Calculating expiry deadlines
----------------------------
diff --git a/include/hw/clock.h b/include/hw/clock.h
index 852c636961e..6382f346569 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -264,4 +264,16 @@ static inline bool clock_is_enabled(const Clock *clk)
return clock_get(clk) != 0;
}
+/**
+ * clock_display_freq: return human-readable representation of clock frequency
+ * @clk: clock
+ *
+ * Return a string which has a human-readable representation of the
+ * clock's frequency, e.g. "33.3 MHz". This is intended for debug
+ * and display purposes.
+ *
+ * The caller is responsible for freeing the string with g_free().
+ */
+char *clock_display_freq(Clock *clk);
+
#endif /* QEMU_HW_CLOCK_H */
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 8c6af223e7c..76b5f468b6e 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -12,6 +12,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "hw/clock.h"
#include "trace.h"
@@ -111,6 +112,11 @@ static void clock_disconnect(Clock *clk)
QLIST_REMOVE(clk, sibling);
}
+char *clock_display_freq(Clock *clk)
+{
+ return freq_to_str(clock_get_hz(clk));
+}
+
static void clock_initfn(Object *obj)
{
Clock *clk = CLOCK(obj);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 832e2548424..fb6c94eaeaf 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -732,11 +732,11 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
}
}
QLIST_FOREACH(ncl, &dev->clocks, node) {
- qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
+ g_autofree char *freq_str = clock_display_freq(ncl->clock);
+ qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
ncl->output ? "out" : "in",
ncl->alias ? " (alias)" : "",
- ncl->name,
- CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
+ ncl->name, freq_str);
}
class = object_get_class(OBJECT(dev));
do {
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns()
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
@ 2020-12-15 15:21 ` Richard Henderson
2020-12-15 19:48 ` Luc Michel
1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2020-12-15 15:21 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Aleksandar Rikalo, Luc Michel, Jiaxun Yang, Philippe Mathieu-Daudé
On 12/15/20 9:09 AM, 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>
> ---
> 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(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] clock: Get rid of clock_get_ns()
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
` (3 preceding siblings ...)
2020-12-15 15:09 ` [PATCH v2 4/4] clock: Define and use new clock_display_freq() Peter Maydell
@ 2020-12-15 15:29 ` Philippe Mathieu-Daudé
2020-12-15 23:08 ` Philippe Mathieu-Daudé
2021-01-01 20:35 ` Philippe Mathieu-Daudé
5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-12-15 15:29 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, Huacai Chen
Cc: Aleksandar Rikalo, Richard Henderson, Jiaxun Yang, Luc Michel
On 12/15/20 4:09 PM, Peter Maydell wrote:
> This patchseries makes some changes to the clock API:
> * Remove clock_get_ns()
> * Add clock_ticks_to_ns() to return number of nanoseconds
> it will take the clock to tick N times
> * clock_display_freq() to return prettily-formatted string
> for showing humans the approximate clock frequency
>
> This is based on discussions we had about these APIs a little while
> back. The core driver here is that the clock objects internally
> store the period in units of 2^-32 ns, so both clock_get_ns() and
> clock_get_hz() are inherently returning a rounded-off result, which
> can be badly inaccurate for fast clocks or if you want to multiply it
> by a large tick count.
>
> Ideally I'd like to get rid of clock_get_hz() as well, but
> that looks trickier than handling clock_get_ns().
>
> Patch 4 borrows a lot of the concept from one of Philippe's that he
> sent out previously.
>
> NB: tested with 'make check' and 'make check-acceptance' only.
Tested using loongson3-virt @2GHz
https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg04036.html
Without your series:
qemu-system-mips64el: target/mips/cpu.c:385: mips_cp0_period_set:
Assertion `env->cp0_count_ns' failed.
Aborted (core dumped)
With: OK (cpu are not displayed in 'info qtree).
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Thanks!
Phil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns()
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
2020-12-15 15:21 ` Richard Henderson
@ 2020-12-15 19:48 ` Luc Michel
1 sibling, 0 replies; 11+ messages in thread
From: Luc Michel @ 2020-12-15 19:48 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Philippe Mathieu-Daudé,
Jiaxun Yang
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] clock: Get rid of clock_get_ns()
2020-12-15 15:29 ` [PATCH v2 0/4] clock: Get rid of clock_get_ns() Philippe Mathieu-Daudé
@ 2020-12-15 23:08 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-12-15 23:08 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, Huacai Chen
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel, Jiaxun Yang
On 12/15/20 4:29 PM, Philippe Mathieu-Daudé wrote:
> On 12/15/20 4:09 PM, Peter Maydell wrote:
>> This patchseries makes some changes to the clock API:
>> * Remove clock_get_ns()
>> * Add clock_ticks_to_ns() to return number of nanoseconds
>> it will take the clock to tick N times
>> * clock_display_freq() to return prettily-formatted string
>> for showing humans the approximate clock frequency
>>
...
>
> Tested using loongson3-virt @2GHz
> https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg04036.html
>
> Without your series:
> qemu-system-mips64el: target/mips/cpu.c:385: mips_cp0_period_set:
> Assertion `env->cp0_count_ns' failed.
> Aborted (core dumped)
>
> With: OK (cpu are not displayed in 'info qtree').
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
I meant:
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
But now I reviewed, so confirming:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Thanks!
Phil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] clock: Get rid of clock_get_ns()
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
` (4 preceding siblings ...)
2020-12-15 15:29 ` [PATCH v2 0/4] clock: Get rid of clock_get_ns() Philippe Mathieu-Daudé
@ 2021-01-01 20:35 ` Philippe Mathieu-Daudé
2021-01-03 13:47 ` Philippe Mathieu-Daudé
5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-01 20:35 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel
On 12/15/20 4:09 PM, Peter Maydell wrote:
> This patchseries makes some changes to the clock API:
> * Remove clock_get_ns()
> * Add clock_ticks_to_ns() to return number of nanoseconds
> it will take the clock to tick N times
> * clock_display_freq() to return prettily-formatted string
> for showing humans the approximate clock frequency
>
> This is based on discussions we had about these APIs a little while
> back. The core driver here is that the clock objects internally
> store the period in units of 2^-32 ns, so both clock_get_ns() and
> clock_get_hz() are inherently returning a rounded-off result, which
> can be badly inaccurate for fast clocks or if you want to multiply it
> by a large tick count.
...
> Peter Maydell (4):
> clock: Introduce clock_ticks_to_ns()
> target/mips: Don't use clock_get_ns() in clock period calculation
> clock: Remove clock_get_ns()
> clock: Define and use new clock_display_freq()
>
> docs/devel/clocks.rst | 51 ++++++++++++++++++++++++++++++++++++----
> include/hw/clock.h | 53 +++++++++++++++++++++++++++++++++++++++---
> hw/core/clock.c | 6 +++++
> softmmu/qdev-monitor.c | 6 ++---
> target/mips/cpu.c | 4 ++--
> 5 files changed, 108 insertions(+), 12 deletions(-)
Until someone else prefers otherwise, I plan to queue this series
via the MIPS tree, as I can add the patch which increases the
Loongson3v frequency to 2GHz on top (the unique machine using
a frequency over 1GHz).
Thanks,
Phil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] clock: Get rid of clock_get_ns()
2021-01-01 20:35 ` Philippe Mathieu-Daudé
@ 2021-01-03 13:47 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-03 13:47 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Aleksandar Rikalo, Richard Henderson, Luc Michel
On 1/1/21 9:35 PM, Philippe Mathieu-Daudé wrote:
> On 12/15/20 4:09 PM, Peter Maydell wrote:
>> This patchseries makes some changes to the clock API:
>> * Remove clock_get_ns()
>> * Add clock_ticks_to_ns() to return number of nanoseconds
>> it will take the clock to tick N times
>> * clock_display_freq() to return prettily-formatted string
>> for showing humans the approximate clock frequency
>>
>> This is based on discussions we had about these APIs a little while
>> back. The core driver here is that the clock objects internally
>> store the period in units of 2^-32 ns, so both clock_get_ns() and
>> clock_get_hz() are inherently returning a rounded-off result, which
>> can be badly inaccurate for fast clocks or if you want to multiply it
>> by a large tick count.
> ...
>> Peter Maydell (4):
>> clock: Introduce clock_ticks_to_ns()
>> target/mips: Don't use clock_get_ns() in clock period calculation
>> clock: Remove clock_get_ns()
>> clock: Define and use new clock_display_freq()
>>
>> docs/devel/clocks.rst | 51 ++++++++++++++++++++++++++++++++++++----
>> include/hw/clock.h | 53 +++++++++++++++++++++++++++++++++++++++---
>> hw/core/clock.c | 6 +++++
>> softmmu/qdev-monitor.c | 6 ++---
>> target/mips/cpu.c | 4 ++--
>> 5 files changed, 108 insertions(+), 12 deletions(-)
>
> Until someone else prefers otherwise, I plan to queue this series
> via the MIPS tree, as I can add the patch which increases the
> Loongson3v frequency to 2GHz on top (the unique machine using
> a frequency over 1GHz).
Thanks, series applied to mips-next.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-01-03 13:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 15:09 [PATCH v2 0/4] clock: Get rid of clock_get_ns() Peter Maydell
2020-12-15 15:09 ` [PATCH v2 1/4] clock: Introduce clock_ticks_to_ns() Peter Maydell
2020-12-15 15:21 ` Richard Henderson
2020-12-15 19:48 ` Luc Michel
2020-12-15 15:09 ` [PATCH v2 2/4] target/mips: Don't use clock_get_ns() in clock period calculation Peter Maydell
2020-12-15 15:09 ` [PATCH v2 3/4] clock: Remove clock_get_ns() Peter Maydell
2020-12-15 15:09 ` [PATCH v2 4/4] clock: Define and use new clock_display_freq() Peter Maydell
2020-12-15 15:29 ` [PATCH v2 0/4] clock: Get rid of clock_get_ns() Philippe Mathieu-Daudé
2020-12-15 23:08 ` Philippe Mathieu-Daudé
2021-01-01 20:35 ` Philippe Mathieu-Daudé
2021-01-03 13:47 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).