[PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[Philippe Mathieu-Daudé]

This series contains few patches resulting from the notes I
took while reviewing Mark ADB series last Sunday, in particular:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg712078.html

I have another patch for hw/input/hid.c but I prefer to hold it
to test it more.

Philippe Mathieu-Daudé (7):
  qemu-common: Briefly document qemu_timedate_diff() unit
  block/qcow2: Document cache_clean_interval field holds seconds
  block/curl: Reduce timer precision to milli-second
  hw/virtio/virtio-balloon: Rename timer field including 'ms' unit
  hw/rtc/m48t59: Reduce timer precision to milli-second
  hw/ipmi/ipmi_bmc_extern: Reduce timer precision to milli-second
  hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

 block/qcow2.h                      |  1 +
 hw/rtc/m48t59-internal.h           |  2 +-
 include/hw/virtio/virtio-balloon.h |  2 +-
 include/hw/watchdog/wdt_aspeed.h   |  2 +-
 include/qemu-common.h              |  1 +
 block/curl.c                       | 15 +++++++--------
 hw/ipmi/ipmi_bmc_extern.c          | 19 +++++++++---------
 hw/rtc/m48t59.c                    | 31 +++++++++++++++---------------
 hw/virtio/virtio-balloon.c         | 14 ++++++++------
 hw/watchdog/wdt_aspeed.c           | 24 ++++++++++++-----------
 softmmu/vl.c                       |  2 +-
 11 files changed, 60 insertions(+), 53 deletions(-)

-- 
2.21.3

[PATCH 1/7] qemu-common: Briefly document qemu_timedate_diff() unit

[Philippe Mathieu-Daudé]

It is not obvious that the qemu_timedate_diff() and
qemu_ref_timedate() functions return seconds. Briefly
document it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu-common.h | 1 +
 softmmu/vl.c          | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index d0142f29ac..e97644710c 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -27,6 +27,7 @@ int qemu_main(int argc, char **argv, char **envp);
 #endif
 
 void qemu_get_timedate(struct tm *tm, int offset);
+/* Returns difference with RTC reference time (in seconds) */
 int qemu_timedate_diff(struct tm *tm);
 
 void *qemu_oom_check(void *ptr);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f669c06ede..215459c7b5 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -737,7 +737,7 @@ void qemu_system_vmstop_request(RunState state)
 }
 
 /***********************************************************/
-/* RTC reference time/date access */
+/* RTC reference time/date access (in seconds) */
 static time_t qemu_ref_timedate(QEMUClockType clock)
 {
     time_t value = qemu_clock_get_ms(clock) / 1000;
-- 
2.21.3

Re: [PATCH 1/7] qemu-common: Briefly document qemu_timedate_diff() unit

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> It is not obvious that the qemu_timedate_diff() and
> qemu_ref_timedate() functions return seconds. Briefly
> document it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu-common.h | 1 +
>  softmmu/vl.c          | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d0142f29ac..e97644710c 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -27,6 +27,7 @@ int qemu_main(int argc, char **argv, char **envp);
>  #endif
>  
>  void qemu_get_timedate(struct tm *tm, int offset);
> +/* Returns difference with RTC reference time (in seconds) */
>  int qemu_timedate_diff(struct tm *tm);

Not this patch's problem: use of int here smells; is it wide enough?

>  
>  void *qemu_oom_check(void *ptr);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f669c06ede..215459c7b5 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -737,7 +737,7 @@ void qemu_system_vmstop_request(RunState state)
>  }
>  
>  /***********************************************************/
> -/* RTC reference time/date access */
> +/* RTC reference time/date access (in seconds) */
>  static time_t qemu_ref_timedate(QEMUClockType clock)
>  {
>      time_t value = qemu_clock_get_ms(clock) / 1000;

time_t is seconds on all systems we support.  Using it for something
other than seconds would be wrong.  The comment feels redundant to me.
But if it helps someone else...

Re: [PATCH 1/7] qemu-common: Briefly document qemu_timedate_diff() unit

[Philippe Mathieu-Daudé]

On 6/18/20 7:47 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> It is not obvious that the qemu_timedate_diff() and
>> qemu_ref_timedate() functions return seconds. Briefly
>> document it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/qemu-common.h | 1 +
>>  softmmu/vl.c          | 2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index d0142f29ac..e97644710c 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -27,6 +27,7 @@ int qemu_main(int argc, char **argv, char **envp);
>>  #endif
>>  
>>  void qemu_get_timedate(struct tm *tm, int offset);
>> +/* Returns difference with RTC reference time (in seconds) */
>>  int qemu_timedate_diff(struct tm *tm);
> 
> Not this patch's problem: use of int here smells; is it wide enough?

I'll add a /* FIXME */ comment.

> 
>>  
>>  void *qemu_oom_check(void *ptr);
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f669c06ede..215459c7b5 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -737,7 +737,7 @@ void qemu_system_vmstop_request(RunState state)
>>  }
>>  
>>  /***********************************************************/
>> -/* RTC reference time/date access */
>> +/* RTC reference time/date access (in seconds) */
>>  static time_t qemu_ref_timedate(QEMUClockType clock)
>>  {
>>      time_t value = qemu_clock_get_ms(clock) / 1000;
> 
> time_t is seconds on all systems we support.  Using it for something
> other than seconds would be wrong.  The comment feels redundant to me.
> But if it helps someone else...

Ah, TIL 'time_t' is the arithmetic time type to represent
the number of seconds since the epoch.

I guess I almost never used it ... (Not something real time
embedded systems care much) :)

So scratch that comment.

> 
> 

[PATCH 2/7] block/qcow2: Document cache_clean_interval field holds seconds

[Philippe Mathieu-Daudé]

It is not obvious the 'cache_clean_interval' field holds
a value expressing seconds. Add a brief comment.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/qcow2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 7ce2c23bdb..fa5c2e64a1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -310,6 +310,7 @@ typedef struct BDRVQcow2State {
     Qcow2Cache* l2_table_cache;
     Qcow2Cache* refcount_block_cache;
     QEMUTimer *cache_clean_timer;
+    /* Interval for cache cleanup timer (in seconds) */
     unsigned cache_clean_interval;
 
     QLIST_HEAD(, QCowL2Meta) cluster_allocs;
-- 
2.21.3

[PATCH 3/7] block/curl: Reduce timer precision to milli-second

[Philippe Mathieu-Daudé]

The current implementation uses nano-second precision,
while the block driver is only used with a milli-second precision.
Simplify by using a milli-second based timer.
Rename the timer 'timer_ms' to have the unit explicit.

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/curl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 6e325901dc..74950373aa 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -119,7 +119,7 @@ typedef struct CURLState
 
 typedef struct BDRVCURLState {
     CURLM *multi;
-    QEMUTimer timer;
+    QEMUTimer timer_ms;
     uint64_t len;
     CURLState states[CURL_NUM_STATES];
     char *url;
@@ -148,11 +148,10 @@ static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
 
     trace_curl_timer_cb(timeout_ms);
     if (timeout_ms == -1) {
-        timer_del(&s->timer);
+        timer_del(&s->timer_ms);
     } else {
-        int64_t timeout_ns = (int64_t)timeout_ms * 1000 * 1000;
-        timer_mod(&s->timer,
-                  qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ns);
+        timer_mod(&s->timer_ms,
+                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + timeout_ms);
     }
     return 0;
 }
@@ -582,7 +581,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
     }
     qemu_mutex_unlock(&s->mutex);
 
-    timer_del(&s->timer);
+    timer_del(&s->timer_ms);
 }
 
 static void curl_attach_aio_context(BlockDriverState *bs,
@@ -590,8 +589,8 @@ static void curl_attach_aio_context(BlockDriverState *bs,
 {
     BDRVCURLState *s = bs->opaque;
 
-    aio_timer_init(new_context, &s->timer,
-                   QEMU_CLOCK_REALTIME, SCALE_NS,
+    aio_timer_init(new_context, &s->timer_ms,
+                   QEMU_CLOCK_REALTIME, SCALE_MS,
                    curl_multi_timeout_do, s);
 
     assert(!s->multi);
-- 
2.21.3

[PATCH 4/7] hw/virtio/virtio-balloon: Rename timer field including 'ms' unit

[Philippe Mathieu-Daudé]

To make code review easier, append the timer unit (milli-seconds)
to its variable name.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/virtio/virtio-balloon.h |  2 +-
 hw/virtio/virtio-balloon.c         | 14 ++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index d49fef00ce..8a85fb1b88 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -50,7 +50,7 @@ typedef struct VirtIOBalloon {
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
-    QEMUTimer *stats_timer;
+    QEMUTimer *stats_timer_ms;
     IOThread *iothread;
     QEMUBH *free_page_bh;
     /*
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 10507b2a43..ad67cd53e4 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -197,16 +197,17 @@ static bool balloon_stats_enabled(const VirtIOBalloon *s)
 static void balloon_stats_destroy_timer(VirtIOBalloon *s)
 {
     if (balloon_stats_enabled(s)) {
-        timer_del(s->stats_timer);
-        timer_free(s->stats_timer);
-        s->stats_timer = NULL;
+        timer_del(s->stats_timer_ms);
+        timer_free(s->stats_timer_ms);
+        s->stats_timer_ms = NULL;
         s->stats_poll_interval = 0;
     }
 }
 
 static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
 {
-    timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
+    timer_mod(s->stats_timer_ms,
+              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
 }
 
 static void balloon_stats_poll_cb(void *opaque)
@@ -315,8 +316,9 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     }
 
     /* create a new timer */
-    g_assert(s->stats_timer == NULL);
-    s->stats_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, balloon_stats_poll_cb, s);
+    g_assert(s->stats_timer_ms == NULL);
+    s->stats_timer_ms = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                     balloon_stats_poll_cb, s);
     s->stats_poll_interval = value;
     balloon_stats_change_timer(s, 0);
 }
-- 
2.21.3

Re: [PATCH 4/7] hw/virtio/virtio-balloon: Rename timer field including 'ms' unit

[David Hildenbrand]

On 16.06.20 09:51, Philippe Mathieu-Daudé wrote:
> To make code review easier, append the timer unit (milli-seconds)
> to its variable name.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/virtio/virtio-balloon.h |  2 +-
>  hw/virtio/virtio-balloon.c         | 14 ++++++++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index d49fef00ce..8a85fb1b88 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -50,7 +50,7 @@ typedef struct VirtIOBalloon {
>      uint64_t stats[VIRTIO_BALLOON_S_NR];
>      VirtQueueElement *stats_vq_elem;
>      size_t stats_vq_offset;
> -    QEMUTimer *stats_timer;
> +    QEMUTimer *stats_timer_ms;
>      IOThread *iothread;
>      QEMUBH *free_page_bh;
>      /*
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 10507b2a43..ad67cd53e4 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -197,16 +197,17 @@ static bool balloon_stats_enabled(const VirtIOBalloon *s)
>  static void balloon_stats_destroy_timer(VirtIOBalloon *s)
>  {
>      if (balloon_stats_enabled(s)) {
> -        timer_del(s->stats_timer);
> -        timer_free(s->stats_timer);
> -        s->stats_timer = NULL;
> +        timer_del(s->stats_timer_ms);
> +        timer_free(s->stats_timer_ms);
> +        s->stats_timer_ms = NULL;
>          s->stats_poll_interval = 0;
>      }
>  }
>  
>  static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
>  {
> -    timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
> +    timer_mod(s->stats_timer_ms,
> +              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
>  }
>  
>  static void balloon_stats_poll_cb(void *opaque)
> @@ -315,8 +316,9 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>      }
>  
>      /* create a new timer */
> -    g_assert(s->stats_timer == NULL);
> -    s->stats_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, balloon_stats_poll_cb, s);
> +    g_assert(s->stats_timer_ms == NULL);
> +    s->stats_timer_ms = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                     balloon_stats_poll_cb, s);
>      s->stats_poll_interval = value;
>      balloon_stats_change_timer(s, 0);
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 5/7] hw/rtc/m48t59: Reduce timer precision to milli-second

[Philippe Mathieu-Daudé]

The current implementation uses nano-second precision,
while the RTC can not be more precise than a milli-second.
Simplify by using a milli-second based timer.
Rename the timer 'alrm_timer_ms' to have the unit explicit.

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/rtc/m48t59-internal.h |  2 +-
 hw/rtc/m48t59.c          | 31 ++++++++++++++++---------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/rtc/m48t59-internal.h b/hw/rtc/m48t59-internal.h
index cd648241e9..f21b603f97 100644
--- a/hw/rtc/m48t59-internal.h
+++ b/hw/rtc/m48t59-internal.h
@@ -49,7 +49,7 @@ typedef struct M48t59State {
     time_t   stop_time;
     /* Alarm & watchdog */
     struct tm alarm;
-    QEMUTimer *alrm_timer;
+    QEMUTimer *alrm_timer_ms;
     QEMUTimer *wd_timer;
     /* NVRAM storage */
     uint8_t *buffer;
diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index 47d48054fd..d2717d00a9 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -89,7 +89,7 @@ static M48txxInfo m48txx_sysbus_info[] = {
 static void alarm_cb (void *opaque)
 {
     struct tm tm;
-    uint64_t next_time;
+    uint64_t next_time_s;
     M48t59State *NVRAM = opaque;
 
     qemu_set_irq(NVRAM->IRQ, 1);
@@ -104,42 +104,43 @@ static void alarm_cb (void *opaque)
             tm.tm_mon = 1;
             tm.tm_year++;
         }
-        next_time = qemu_timedate_diff(&tm) - NVRAM->time_offset;
+        next_time_s = qemu_timedate_diff(&tm) - NVRAM->time_offset;
     } else if ((NVRAM->buffer[0x1FF5] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF4] & 0x80) == 0 &&
 	       (NVRAM->buffer[0x1FF3] & 0x80) == 0 &&
 	       (NVRAM->buffer[0x1FF2] & 0x80) == 0) {
         /* Repeat once a day */
-        next_time = 24 * 60 * 60;
+        next_time_s = 24 * 60 * 60;
     } else if ((NVRAM->buffer[0x1FF5] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF4] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF3] & 0x80) == 0 &&
 	       (NVRAM->buffer[0x1FF2] & 0x80) == 0) {
         /* Repeat once an hour */
-        next_time = 60 * 60;
+        next_time_s = 60 * 60;
     } else if ((NVRAM->buffer[0x1FF5] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF4] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF3] & 0x80) != 0 &&
 	       (NVRAM->buffer[0x1FF2] & 0x80) == 0) {
         /* Repeat once a minute */
-        next_time = 60;
+        next_time_s = 60;
     } else {
         /* Repeat once a second */
-        next_time = 1;
+        next_time_s = 1;
     }
-    timer_mod(NVRAM->alrm_timer, qemu_clock_get_ns(rtc_clock) +
-                    next_time * 1000);
+    timer_mod(NVRAM->alrm_timer_ms,
+              qemu_clock_get_ms(rtc_clock) + next_time_s *
+                                             NANOSECONDS_PER_SECOND / SCALE_MS);
     qemu_set_irq(NVRAM->IRQ, 0);
 }
 
 static void set_alarm(M48t59State *NVRAM)
 {
     int diff;
-    if (NVRAM->alrm_timer != NULL) {
-        timer_del(NVRAM->alrm_timer);
+    if (NVRAM->alrm_timer_ms != NULL) {
+        timer_del(NVRAM->alrm_timer_ms);
         diff = qemu_timedate_diff(&NVRAM->alarm) - NVRAM->time_offset;
         if (diff > 0)
-            timer_mod(NVRAM->alrm_timer, diff * 1000);
+            timer_mod(NVRAM->alrm_timer_ms, diff * 1000);
     }
 }
 
@@ -539,9 +540,9 @@ void m48t59_reset_common(M48t59State *NVRAM)
 {
     NVRAM->addr = 0;
     NVRAM->lock = 0;
-    if (NVRAM->alrm_timer != NULL)
-        timer_del(NVRAM->alrm_timer);
-
+    if (NVRAM->alrm_timer_ms != NULL) {
+        timer_del(NVRAM->alrm_timer_ms);
+    }
     if (NVRAM->wd_timer != NULL)
         timer_del(NVRAM->wd_timer);
 }
@@ -603,7 +604,7 @@ void m48t59_realize_common(M48t59State *s, Error **errp)
 {
     s->buffer = g_malloc0(s->size);
     if (s->model == 59) {
-        s->alrm_timer = timer_new_ns(rtc_clock, &alarm_cb, s);
+        s->alrm_timer_ms = timer_new_ms(rtc_clock, &alarm_cb, s);
         s->wd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &watchdog_cb, s);
     }
     qemu_get_timedate(&s->alarm, 0);
-- 
2.21.3

Re: [PATCH 5/7] hw/rtc/m48t59: Reduce timer precision to milli-second

[Richard Henderson]

On 6/16/20 12:51 AM, Philippe Mathieu-Daudé wrote:
> +    timer_mod(NVRAM->alrm_timer_ms,
> +              qemu_clock_get_ms(rtc_clock) + next_time_s *
> +                                             NANOSECONDS_PER_SECOND / SCALE_MS);

I'm not keen on this last.

I would much prefer a MILLISECONDS_PER_SECOND define.  Should we in fact change
the units of the input...


r~

[PATCH 6/7] hw/ipmi/ipmi_bmc_extern: Reduce timer precision to milli-second

[Philippe Mathieu-Daudé]

The current implementation uses nano-second precision, while
the device can not be more precise than a milli-second.
Simplify by using a milli-second based timer.
Rename the timer 'extern_timer_ms' to have the unit explicit.

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ipmi/ipmi_bmc_extern.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index f9a13e0a44..441d3ed18e 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -81,7 +81,7 @@ typedef struct IPMIBmcExtern {
     unsigned int outpos;
     unsigned int outlen;
 
-    struct QEMUTimer *extern_timer;
+    QEMUTimer *extern_timer_ms;
 
     /* A reset event is pending to be sent upstream. */
     bool send_reset;
@@ -112,8 +112,8 @@ static void continue_send(IPMIBmcExtern *ibe)
     }
     if (ibe->outpos < ibe->outlen) {
         /* Not fully transmitted, try again in a 10ms */
-        timer_mod_ns(ibe->extern_timer,
-                     qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 10000000);
+        timer_mod(ibe->extern_timer_ms,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 10);
     } else {
         /* Sent */
         ibe->outlen = 0;
@@ -137,8 +137,8 @@ static void continue_send(IPMIBmcExtern *ibe)
 
         if (ibe->waiting_rsp) {
             /* Make sure we get a response within 4 seconds. */
-            timer_mod_ns(ibe->extern_timer,
-                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 4000000000ULL);
+            timer_mod(ibe->extern_timer_ms,
+                      qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 4 * 1000);
         }
     }
     return;
@@ -303,7 +303,7 @@ static void handle_msg(IPMIBmcExtern *ibe)
         ibe->inpos--; /* Remove checkum */
     }
 
-    timer_del(ibe->extern_timer);
+    timer_del(ibe->extern_timer_ms);
     ibe->waiting_rsp = false;
     k->handle_rsp(ibe->parent.intf, ibe->inbuf[0], ibe->inbuf + 1, ibe->inpos - 1);
 }
@@ -502,7 +502,8 @@ static void ipmi_bmc_extern_init(Object *obj)
 {
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
 
-    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
+    ibe->extern_timer_ms = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                        extern_timeout, ibe);
     vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
 }
 
@@ -510,8 +511,8 @@ static void ipmi_bmc_extern_finalize(Object *obj)
 {
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
 
-    timer_del(ibe->extern_timer);
-    timer_free(ibe->extern_timer);
+    timer_del(ibe->extern_timer_ms);
+    timer_free(ibe->extern_timer_ms);
 }
 
 static Property ipmi_bmc_extern_properties[] = {
-- 
2.21.3

[PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Philippe Mathieu-Daudé]

The current implementation uses nano-second precision, while
the watchdog can not be more precise than a micro-second.
Simplify by using a micro-second based timer.
Rename the timer 'timer_us' to have the unit explicit.

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/watchdog/wdt_aspeed.h |  2 +-
 hw/watchdog/wdt_aspeed.c         | 24 +++++++++++++-----------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index 819c22993a..e76a493788 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -25,7 +25,7 @@
 typedef struct AspeedWDTState {
     /*< private >*/
     SysBusDevice parent_obj;
-    QEMUTimer *timer;
+    QEMUTimer *timer_us;
 
     /*< public >*/
     MemoryRegion iomem;
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 6352ba1b0e..3fcb20f72b 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -98,23 +98,24 @@ static void aspeed_wdt_reload(AspeedWDTState *s)
     uint64_t reload;
 
     if (!(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK)) {
-        reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
+        reload = muldiv64(s->regs[WDT_RELOAD_VALUE],
+                          NANOSECONDS_PER_SECOND / SCALE_US,
                           s->pclk_freq);
     } else {
-        reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+        reload = s->regs[WDT_RELOAD_VALUE];
     }
 
     if (aspeed_wdt_is_enabled(s)) {
-        timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+        timer_mod(s->timer_us, qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + reload);
     }
 }
 
 static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
 {
-    uint64_t reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+    uint64_t reload = s->regs[WDT_RELOAD_VALUE];
 
     if (aspeed_wdt_is_enabled(s)) {
-        timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+        timer_mod(s->timer_us, qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + reload);
     }
 }
 
@@ -149,7 +150,7 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
             awc->wdt_reload(s);
         } else if (!enable && aspeed_wdt_is_enabled(s)) {
             s->regs[WDT_CTRL] = data;
-            timer_del(s->timer);
+            timer_del(s->timer_us);
         }
         break;
     case WDT_RESET_WIDTH:
@@ -189,7 +190,7 @@ static const VMStateDescription vmstate_aspeed_wdt = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_TIMER_PTR(timer, AspeedWDTState),
+        VMSTATE_TIMER_PTR(timer_us, AspeedWDTState),
         VMSTATE_UINT32_ARRAY(regs, AspeedWDTState, ASPEED_WDT_REGS_MAX),
         VMSTATE_END_OF_LIST()
     }
@@ -214,7 +215,7 @@ static void aspeed_wdt_reset(DeviceState *dev)
     s->regs[WDT_CTRL] = 0;
     s->regs[WDT_RESET_WIDTH] = 0xFF;
 
-    timer_del(s->timer);
+    timer_del(s->timer_us);
 }
 
 static void aspeed_wdt_timer_expired(void *dev)
@@ -224,7 +225,7 @@ static void aspeed_wdt_timer_expired(void *dev)
 
     /* Do not reset on SDRAM controller reset */
     if (s->scu->regs[reset_ctrl_reg] & SCU_RESET_SDRAM) {
-        timer_del(s->timer);
+        timer_del(s->timer_us);
         s->regs[WDT_CTRL] = 0;
         return;
     }
@@ -232,7 +233,7 @@ static void aspeed_wdt_timer_expired(void *dev)
     qemu_log_mask(CPU_LOG_RESET, "Watchdog timer %" HWADDR_PRIx " expired.\n",
                   s->iomem.addr);
     watchdog_perform_action();
-    timer_del(s->timer);
+    timer_del(s->timer_us);
 }
 
 #define PCLK_HZ 24000000
@@ -244,7 +245,8 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
 
     assert(s->scu);
 
-    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, aspeed_wdt_timer_expired, dev);
+    s->timer_us = timer_new_us(QEMU_CLOCK_VIRTUAL,
+                               aspeed_wdt_timer_expired, dev);
 
     /* FIXME: This setting should be derived from the SCU hw strapping
      * register SCU70
-- 
2.21.3

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Andrew Jeffery]

On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote:
> The current implementation uses nano-second precision, while
> the watchdog can not be more precise than a micro-second.

What's the basis for this assertion? It's true for the AST2500 and AST2600, but 
the AST2400 can run the watchdog from either a 1MHz clock source or the APB 
clock (which must be at least 16.5MHz on palmetto). The reset state on the
AST2400 configures the watchdog for the APB clock rate.

The Linux driver will eventually configure the watchdog for 1MHz mode
regardless so perhaps the AST2400 reset state is a bit of a corner case, but
I feel the assertion should be watered down a bit?

Andrew

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Philippe Mathieu-Daudé]

Hi Andrew,

On 6/17/20 3:18 AM, Andrew Jeffery wrote:
> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote:
>> The current implementation uses nano-second precision, while
>> the watchdog can not be more precise than a micro-second.
> 
> What's the basis for this assertion? It's true for the AST2500 and AST2600, but 
> the AST2400 can run the watchdog from either a 1MHz clock source or the APB 
> clock (which must be at least 16.5MHz on palmetto). The reset state on the
> AST2400 configures the watchdog for the APB clock rate.
> 
> The Linux driver will eventually configure the watchdog for 1MHz mode
> regardless so perhaps the AST2400 reset state is a bit of a corner case, but
> I feel the assertion should be watered down a bit?

What about this description?

"The current implementation uses nano-second precision, but
 is not more precise than micro-second precision.
 Simplify by using a micro-second based timer.
 Rename the timer 'timer_us' to have the unit explicit."

> 
> Andrew
> 

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Andrew Jeffery]

On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote:
> Hi Andrew,
> 
> On 6/17/20 3:18 AM, Andrew Jeffery wrote:
> > On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote:
> >> The current implementation uses nano-second precision, while
> >> the watchdog can not be more precise than a micro-second.
> > 
> > What's the basis for this assertion? It's true for the AST2500 and AST2600, but 
> > the AST2400 can run the watchdog from either a 1MHz clock source or the APB 
> > clock (which must be at least 16.5MHz on palmetto). The reset state on the
> > AST2400 configures the watchdog for the APB clock rate.
> > 
> > The Linux driver will eventually configure the watchdog for 1MHz mode
> > regardless so perhaps the AST2400 reset state is a bit of a corner case, but
> > I feel the assertion should be watered down a bit?
> 
> What about this description?
> 
> "The current implementation uses nano-second precision, but
>  is not more precise than micro-second precision.
>  Simplify by using a micro-second based timer.
>  Rename the timer 'timer_us' to have the unit explicit."

So is this a limitation of QEMUTimer? I was establishing that the hardware can 
operate at greater than 1 micro-second precision.

Andrew

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Philippe Mathieu-Daudé]

On 6/22/20 2:21 AM, Andrew Jeffery wrote:
> On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote:
>> Hi Andrew,
>>
>> On 6/17/20 3:18 AM, Andrew Jeffery wrote:
>>> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote:
>>>> The current implementation uses nano-second precision, while
>>>> the watchdog can not be more precise than a micro-second.
>>>
>>> What's the basis for this assertion? It's true for the AST2500 and AST2600, but 
>>> the AST2400 can run the watchdog from either a 1MHz clock source or the APB 
>>> clock (which must be at least 16.5MHz on palmetto). The reset state on the
>>> AST2400 configures the watchdog for the APB clock rate.
>>>
>>> The Linux driver will eventually configure the watchdog for 1MHz mode
>>> regardless so perhaps the AST2400 reset state is a bit of a corner case, but
>>> I feel the assertion should be watered down a bit?
>>
>> What about this description?
>>
>> "The current implementation uses nano-second precision, but
>>  is not more precise than micro-second precision.
>>  Simplify by using a micro-second based timer.
>>  Rename the timer 'timer_us' to have the unit explicit."
> 
> So is this a limitation of QEMUTimer? I was establishing that the hardware can 
> operate at greater than 1 micro-second precision.

No, I misread your comment about the AST2400 timer which can run
at more than 1Mhz.

The QEMUTimer doesn't have a such limitation; this patch
aimed to simplify the code for reviewers, but you proved
it incorrect, so let's disregard it.

Thanks for your careful review!

> 
> Andrew
> 

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Andrew Jeffery]

On Mon, 22 Jun 2020, at 18:13, Philippe Mathieu-Daudé wrote:
> On 6/22/20 2:21 AM, Andrew Jeffery wrote:
> > On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote:
> >> Hi Andrew,
> >>
> >> On 6/17/20 3:18 AM, Andrew Jeffery wrote:
> >>> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote:
> >>>> The current implementation uses nano-second precision, while
> >>>> the watchdog can not be more precise than a micro-second.
> >>>
> >>> What's the basis for this assertion? It's true for the AST2500 and AST2600, but 
> >>> the AST2400 can run the watchdog from either a 1MHz clock source or the APB 
> >>> clock (which must be at least 16.5MHz on palmetto). The reset state on the
> >>> AST2400 configures the watchdog for the APB clock rate.
> >>>
> >>> The Linux driver will eventually configure the watchdog for 1MHz mode
> >>> regardless so perhaps the AST2400 reset state is a bit of a corner case, but
> >>> I feel the assertion should be watered down a bit?
> >>
> >> What about this description?
> >>
> >> "The current implementation uses nano-second precision, but
> >>  is not more precise than micro-second precision.
> >>  Simplify by using a micro-second based timer.
> >>  Rename the timer 'timer_us' to have the unit explicit."
> > 
> > So is this a limitation of QEMUTimer? I was establishing that the hardware can 
> > operate at greater than 1 micro-second precision.
> 
> No, I misread your comment about the AST2400 timer which can run
> at more than 1Mhz.
> 
> The QEMUTimer doesn't have a such limitation; this patch
> aimed to simplify the code for reviewers, but you proved
> it incorrect, so let's disregard it.
> 
> Thanks for your careful review!

Ah, great, I was wondering where my misunderstanding was.

Thanks for clearing that up.

Andrew

Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second

[Richard Henderson]

On 6/16/20 12:51 AM, Philippe Mathieu-Daudé wrote:
> -        reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
> +        reload = muldiv64(s->regs[WDT_RELOAD_VALUE],
> +                          NANOSECONDS_PER_SECOND / SCALE_US,
>                            s->pclk_freq);

Similarly, I would prefer MICROSECONDS_PER_SECOND.  Though again, I don't see
what we gain from changing units.


r~

Re: [PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[Paolo Bonzini]

On 16/06/20 09:51, Philippe Mathieu-Daudé wrote:
> This series contains few patches resulting from the notes I
> took while reviewing Mark ADB series last Sunday, in particular:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg712078.html
> 
> I have another patch for hw/input/hid.c but I prefer to hold it
> to test it more.

This is in principle a very good idea; however, util/qemu-timer.c does
not use the scale to coalesce low-precision timers with nearby
high-precision ones.

Paolo

Re: [PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[Philippe Mathieu-Daudé]

On 6/18/20 2:23 PM, Paolo Bonzini wrote:
> On 16/06/20 09:51, Philippe Mathieu-Daudé wrote:
>> This series contains few patches resulting from the notes I
>> took while reviewing Mark ADB series last Sunday, in particular:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg712078.html
>>
>> I have another patch for hw/input/hid.c but I prefer to hold it
>> to test it more.
> 
> This is in principle a very good idea; however, util/qemu-timer.c does
> not use the scale to coalesce low-precision timers with nearby
> high-precision ones.

IOW this doesn't reduce the pressure, but simply makes the code easier?
Only the cover mentions 'pressure', so maybe the patches can still be
reviewed/queued in their current state?

Re: [PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[Paolo Bonzini]

On 18/06/20 14:26, Philippe Mathieu-Daudé wrote:
>> This is in principle a very good idea; however, util/qemu-timer.c does
>> not use the scale to coalesce low-precision timers with nearby
>> high-precision ones.
> IOW this doesn't reduce the pressure, but simply makes the code easier?

Easier, or harder depending on the point of view.  The reason why scale
exists is just because QEMU_CLOCK_REALTIME used to be millisecond based;
having to scale based on the clock was really ugly code, and furthermore
the scale provides an easy way to switch timers from one clock to
another without having to modify every deadline computation.

One might argue that the scale adds to QEMU's cognitive weight for
little benefit.  You might counter-argue that having to scale up to
nanoseconds every time is a pain in the ass, and I am not sure which
side I actually agree with.

> Only the cover mentions 'pressure', so maybe the patches can still be
> reviewed/queued in their current state?

Yes, of course.

Paolo

Re: [PATCH 0/7] misc: Reduce QEMUTimer pressure by using lower precision when possible

[Richard Henderson]

On 6/16/20 12:51 AM, Philippe Mathieu-Daudé wrote:
> This series contains few patches resulting from the notes I
> took while reviewing Mark ADB series last Sunday, in particular:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg712078.html
> 
> I have another patch for hw/input/hid.c but I prefer to hold it
> to test it more.

How does this reduce pressure?  The resolution of the timer doesn't seem to be
affected by these patches, only the precision of the inputs.

It seems like standardizing on nanoseconds makes some things easier, and allows
the resolution of the implementation to vary to a much larger degree.


r~