[Qemu-devel] [PATCH 0/3] Introduce attributes for timers subsystem and remove QEMU_CLOCK_VIRTUAL_EXT clock type

[Qemu-devel] [PATCH 0/3] Introduce attributes for timers subsystem and remove QEMU_CLOCK_VIRTUAL_EXT clock type

[Artem Pisarenko]

Recent patches from series [PATCH v6] "Fixing record/replay and adding reverse debugging" introduced new clock type QEMU_CLOCK_VIRTUAL_EXT and replaced virtual timers in some external subsystems with it.
This resulted in small change to existing behavior, which I consider to be unacceptable.
Processing of virtual timers, belonging to new clock type, was kicked off to main loop, which made them asynchronous with vCPU thread and, in icount mode, with whole guest execution. This breaks expected determinism in non-record/replay icount mode of emulation where these "external subsystems" are isolated from host (i.e. they external only to guest core, not to emulation environment).

Example for slirp ("user" backend for network device):
User runs qemu in icount mode with rtc clock=vm without any external communication interfaces but with "-netdev user,restrict=on". It expects deterministic execution, because network services are emulated inside qemu and isolated from host. There are no reasons to get reply from DHCP server with different delay or something like that.

These series of patches revert those commits and reimplement their modifications in a better way.

Current implementation of timers/clock processing is confusing (at least for me) because of exceptions from design concept behind them, introduced by icount mode (which adds QEMU_CLOCK_VIRTUAL_RT). Adding QEMU_CLOCK_VIRTUAL_EXT just made things even more complicated. I consider these "alternative" virtual clocks to be some kind of hacks being convinient only to authors of relevant qemu features. Lets don't touch fundamental clock types and keep them orthogonal to special cases of timers handling.

As far as I understand, original intention of author was just to make optimization in replay log by avoiding storing extra events which don't change guest state directly. Indeed, for example, ipv6 icmp timer in slirp does things which external to guest core and ends with sending network packet to guest, but record/replay will anyway catch event, corresponding to packet reception in guest network frontend, and store it to replay log, so there are no need in making checkpoint for corresponding clock when that timer fires.
If so, then we just need to skip checkpoints for clock values, when only these specific timers are run. It is individual timers which are specific, not clock.
Adding some kind of attribute/flag/property to individual timer allows any special qemu feature (such as record/replay) to inspect it in any place of code and handle as needed. This design acheives less dependencies, more transparency and has more intuitive and clear sense. For record/replay feature it's acheived with 'EXTERNAL' attribute. The only drawback is that it required to add extra cycle of traversal through active timer list in timerlist_run_timers() function (see patch 3), but I've optimized it in a way that performance degradation expected to be very small and appear only in record/replay mode.

P.S. I've tried to test record/replay with slirp, but in replay mode qemu stucks at guest linux boot after "Configuring network interfaces..." message, where DHCP communication takes place. It's broken in a same way both in master and master with reverted commits being fixed.


Artem Pisarenko (3):
  Revert some patches from recent series [PATCH v6] "Fixing
    record/replay and adding reverse debugging", which introduced new
    virtual clock type for use in external subsystems.     These changes
    breaks desired behavior in non-record/replay usage scenarios.
  Introduce attributes to qemu timer subsystem
  Restores record/replay behavior related to special virtual clock
    processing for timers used in external subsystems.

 include/block/aio.h         |  50 +++++++++++++++++-
 include/qemu/coroutine.h    |   5 +-
 include/qemu/timer.h        | 125 ++++++++++++++++++++++++++++++++++++--------
 slirp/ip6_icmp.c            |   9 ++--
 tests/ptimer-test-stubs.c   |   7 +--
 ui/input.c                  |   9 ++--
 util/qemu-coroutine-sleep.c |   6 ++-
 util/qemu-timer.c           |  81 +++++++++++++++++++---------
 8 files changed, 227 insertions(+), 65 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] Revert some patches from recent series [PATCH v6] "Fixing record/replay and adding reverse debugging", which introduced new virtual clock type for use in external subsystems. These changes breaks desired behavior in non-record/replay usage scenarios.

[Artem Pisarenko]

This reverts commit 87f4fe7653baf55b5c2f2753fe6003f473c07342.
This reverts commit 775a412bf83f6bc0c5c02091ee06cf649b34c593.
This reverts commit 9888091404a702d7ec79d51b088d994b9fc121bd.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---
 include/qemu/timer.h | 9 ---------
 slirp/ip6_icmp.c     | 7 +++----
 ui/input.c           | 8 ++++----
 util/qemu-timer.c    | 2 --
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index a005ed2..39ea907 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -42,14 +42,6 @@
  * In icount mode, this clock counts nanoseconds while the virtual
  * machine is running.  It is used to increase @QEMU_CLOCK_VIRTUAL
  * while the CPUs are sleeping and thus not executing instructions.
- *
- * @QEMU_CLOCK_VIRTUAL_EXT: virtual clock for external subsystems
- *
- * The virtual clock only runs during the emulation. It stops
- * when the virtual machine is stopped. The timers for this clock
- * do not recorded in rr mode, therefore this clock could be used
- * for the subsystems that operate outside the guest core.
- *
  */
 
 typedef enum {
@@ -57,7 +49,6 @@ typedef enum {
     QEMU_CLOCK_VIRTUAL = 1,
     QEMU_CLOCK_HOST = 2,
     QEMU_CLOCK_VIRTUAL_RT = 3,
-    QEMU_CLOCK_VIRTUAL_EXT = 4,
     QEMU_CLOCK_MAX
 } QEMUClockType;
 
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 3f41187..ee333d0 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -17,7 +17,7 @@ static void ra_timer_handler(void *opaque)
 {
     Slirp *slirp = opaque;
     timer_mod(slirp->ra_timer,
-              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_EXT) + NDP_Interval);
+              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + NDP_Interval);
     ndp_send_ra(slirp);
 }
 
@@ -27,10 +27,9 @@ void icmp6_init(Slirp *slirp)
         return;
     }
 
-    slirp->ra_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_EXT,
-                                   ra_timer_handler, slirp);
+    slirp->ra_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, ra_timer_handler, slirp);
     timer_mod(slirp->ra_timer,
-              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_EXT) + NDP_Interval);
+              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + NDP_Interval);
 }
 
 void icmp6_cleanup(Slirp *slirp)
diff --git a/ui/input.c b/ui/input.c
index dd7f6d7..51b1019 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -271,7 +271,7 @@ static void qemu_input_queue_process(void *opaque)
         item = QTAILQ_FIRST(queue);
         switch (item->type) {
         case QEMU_INPUT_QUEUE_DELAY:
-            timer_mod(item->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_EXT)
+            timer_mod(item->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)
                       + item->delay_ms);
             return;
         case QEMU_INPUT_QUEUE_EVENT:
@@ -301,7 +301,7 @@ static void qemu_input_queue_delay(struct QemuInputEventQueueHead *queue,
     queue_count++;
 
     if (start_timer) {
-        timer_mod(item->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_EXT)
+        timer_mod(item->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)
                   + item->delay_ms);
     }
 }
@@ -448,8 +448,8 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
     }
 
     if (!kbd_timer) {
-        kbd_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_EXT,
-                                 qemu_input_queue_process, &kbd_queue);
+        kbd_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, qemu_input_queue_process,
+                                 &kbd_queue);
     }
     if (queue_count < queue_limit) {
         qemu_input_queue_delay(&kbd_queue, kbd_timer,
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index eb60d8f..86bfe84 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -496,7 +496,6 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
 
     switch (timer_list->clock->type) {
     case QEMU_CLOCK_REALTIME:
-    case QEMU_CLOCK_VIRTUAL_EXT:
         break;
     default:
     case QEMU_CLOCK_VIRTUAL:
@@ -598,7 +597,6 @@ int64_t qemu_clock_get_ns(QEMUClockType type)
         return get_clock();
     default:
     case QEMU_CLOCK_VIRTUAL:
-    case QEMU_CLOCK_VIRTUAL_EXT:
         if (use_icount) {
             return cpu_get_icount();
         } else {
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

Attributes are simple flags, associated with individual timers for their whole lifetime.
They intended to be used to mark individual timers for special handling by various qemu features operating at qemu core level.
Existing timer, aio and coroutine interface extended with attribute-enabled variants of functions, which create/initialize timers.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---

Notes:
    Conversion and association between QEMUTimerAttrBit and accessor macro are dumb.
    Maybe better alternatives exist (like QFlags in Qt framework) or existing qemu code may be reused, if any.
    Attributes also may be better named as flags, but they looks like something volatile, whereas 'attribute' expresses constant nature better.

 include/block/aio.h         |  50 +++++++++++++++++++-
 include/qemu/coroutine.h    |   5 +-
 include/qemu/timer.h        | 110 ++++++++++++++++++++++++++++++++++++++------
 tests/ptimer-test-stubs.c   |   7 +--
 util/qemu-coroutine-sleep.c |   6 ++-
 util/qemu-timer.c           |  12 +++--
 6 files changed, 165 insertions(+), 25 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index f08630c..a6be3fb 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -407,10 +407,35 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, QEMUClockType type,
                                        int scale,
                                        QEMUTimerCB *cb, void *opaque)
 {
-    return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
+    return timer_new_a_tl(ctx->tlg.tl[type], scale, 0, cb, opaque);
 }
 
 /**
+ * aio_timer_new_a:
+ * @ctx: the aio context
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Allocate a new timer (with attributes) attached to the context @ctx.
+ * The function is responsible for memory allocation.
+ *
+ * The preferred interface is aio_timer_init. Use that
+ * unless you really need dynamic memory allocation.
+ *
+ * Returns: a pointer to the new timer
+ */
+static inline QEMUTimer *aio_timer_new_a(AioContext *ctx, QEMUClockType type,
+                                         int scale, int attributes,
+                                         QEMUTimerCB *cb, void *opaque)
+{
+    return timer_new_a_tl(ctx->tlg.tl[type], scale, attributes, cb, opaque);
+}
+
+
+/**
  * aio_timer_init:
  * @ctx: the aio context
  * @ts: the timer
@@ -427,7 +452,28 @@ static inline void aio_timer_init(AioContext *ctx,
                                   int scale,
                                   QEMUTimerCB *cb, void *opaque)
 {
-    timer_init_tl(ts, ctx->tlg.tl[type], scale, cb, opaque);
+    timer_init_a_tl(ts, ctx->tlg.tl[type], scale, 0, cb, opaque);
+}
+
+/**
+ * aio_timer_init_a:
+ * @ctx: the aio context
+ * @ts: the timer
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Initialise a new timer (with attributes) attached to the context @ctx.
+ * The caller is responsible for memory allocation.
+ */
+static inline void aio_timer_init_a(AioContext *ctx,
+                                    QEMUTimer *ts, QEMUClockType type,
+                                    int scale, int attributes,
+                                    QEMUTimerCB *cb, void *opaque)
+{
+    timer_init_a_tl(ts, ctx->tlg.tl[type], scale, attributes, cb, opaque);
 }
 
 /**
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9801e7f..cffc2b2 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -276,7 +276,10 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
 /**
  * Yield the coroutine for a given duration
  */
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
+#define qemu_co_sleep_ns(type, ns) \
+    qemu_co_sleep_a_ns(type, 0, ns)
+void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
+                                     int64_t ns);
 
 /**
  * Yield until a file descriptor becomes readable
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 39ea907..031e3a1 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,6 +52,28 @@ typedef enum {
     QEMU_CLOCK_MAX
 } QEMUClockType;
 
+/**
+ * QEMU Timer attributes:
+ *
+ * An individual timer may be assigned with one or multiple attributes when
+ * initialized.
+ * Attribute is a static flag, meaning that timer has corresponding property.
+ * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
+ * which used to initialize timer, stored to 'attributes' member and can be
+ * retrieved externally with timer_get_attributes() call.
+ * Values of QEMUTimerAttrBit aren't used directly,
+ * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
+ * where 'id' is a unique part of attribute identifier.
+ *
+ * No attributes defined currently.
+ */
+
+typedef enum {
+    /* none */
+} QEMUTimerAttrBit;
+
+#define QEMU_TIMER_ATTR(id) (1 << QEMU_TIMER_ATTRBIT_ ## id)
+
 typedef struct QEMUTimerList QEMUTimerList;
 
 struct QEMUTimerListGroup {
@@ -67,6 +89,7 @@ struct QEMUTimer {
     QEMUTimerCB *cb;
     void *opaque;
     QEMUTimer *next;
+    int attributes;
     int scale;
 };
 
@@ -418,10 +441,11 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup *tlg);
  */
 
 /**
- * timer_init_tl:
+ * timer_init_a_tl:
  * @ts: the timer to be initialised
  * @timer_list: the timer list to attach the timer to
  * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
@@ -431,9 +455,9 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup *tlg);
  * You need not call an explicit deinit call. Simply make
  * sure it is not on a list with timer_del.
  */
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque);
+void timer_init_a_tl(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque);
 
 /**
  * timer_init:
@@ -452,7 +476,29 @@ void timer_init_tl(QEMUTimer *ts,
 static inline void timer_init(QEMUTimer *ts, QEMUClockType type, int scale,
                               QEMUTimerCB *cb, void *opaque)
 {
-    timer_init_tl(ts, main_loop_tlg.tl[type], scale, cb, opaque);
+    timer_init_a_tl(ts, main_loop_tlg.tl[type], scale, 0, cb, opaque);
+}
+
+/**
+ * timer_init_a:
+ * @ts: the timer to be initialised
+ * @type: the clock to associate with the timer
+ * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to call when the timer expires
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Initialize a timer with the given scale and attributes on the default
+ * timer list associated with the clock.
+ *
+ * You need not call an explicit deinit call. Simply make
+ * sure it is not on a list with timer_del.
+ */
+static inline void timer_init_a(QEMUTimer *ts, QEMUClockType type,
+                                int scale, int attributes,
+                                QEMUTimerCB *cb, void *opaque)
+{
+    timer_init_a_tl(ts, main_loop_tlg.tl[type], scale, attributes, cb, opaque);
 }
 
 /**
@@ -513,9 +559,10 @@ static inline void timer_init_ms(QEMUTimer *ts, QEMUClockType type,
 }
 
 /**
- * timer_new_tl:
+ * timer_new_a_tl:
  * @timer_list: the timer list to attach the timer to
  * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
@@ -527,13 +574,13 @@ static inline void timer_init_ms(QEMUTimer *ts, QEMUClockType type,
  *
  * Returns: a pointer to the timer
  */
-static inline QEMUTimer *timer_new_tl(QEMUTimerList *timer_list,
-                                      int scale,
-                                      QEMUTimerCB *cb,
-                                      void *opaque)
+static inline QEMUTimer *timer_new_a_tl(QEMUTimerList *timer_list,
+                                        int scale, int attributes,
+                                        QEMUTimerCB *cb,
+                                        void *opaque)
 {
     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
-    timer_init_tl(ts, timer_list, scale, cb, opaque);
+    timer_init_a_tl(ts, timer_list, scale, attributes, cb, opaque);
     return ts;
 }
 
@@ -544,8 +591,8 @@ static inline QEMUTimer *timer_new_tl(QEMUTimerList *timer_list,
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
- * Create a new timer and associate it with the default
- * timer list for the clock type @type.
+ * Create a new timer with the given scale,
+ * and associate it with the default timer list for the clock type @type.
  *
  * The default timer list has one special feature: in icount mode,
  * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
@@ -558,7 +605,34 @@ static inline QEMUTimer *timer_new_tl(QEMUTimerList *timer_list,
 static inline QEMUTimer *timer_new(QEMUClockType type, int scale,
                                    QEMUTimerCB *cb, void *opaque)
 {
-    return timer_new_tl(main_loop_tlg.tl[type], scale, cb, opaque);
+    return timer_new_a_tl(main_loop_tlg.tl[type], scale, 0, cb, opaque);
+}
+
+/**
+ * timer_new_a:
+ * @type: the clock type to use
+ * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to be called when the timer expires
+ * @opaque: the opaque pointer to be passed to the callback
+ *
+ * Create a new timer with the given scale and attributes,
+ * and associate it with the default timer list for the clock type @type.
+ *
+ * The default timer list has one special feature: in icount mode,
+ * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
+ * not true of other timer lists, which are typically associated
+ * with an AioContext---each of them runs its timer callbacks in its own
+ * AioContext thread.
+ *
+ * Returns: a pointer to the timer
+ */
+static inline QEMUTimer *timer_new_a(QEMUClockType type,
+                                     int scale, int attributes,
+                                     QEMUTimerCB *cb, void *opaque)
+{
+    return timer_new_a_tl(main_loop_tlg.tl[type],
+                          scale, attributes, cb, opaque);
 }
 
 /**
@@ -631,6 +705,14 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType type, QEMUTimerCB *cb,
 }
 
 /**
+ * timer_get_attributes:
+ * @ts: the timer
+ *
+ * Return 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values
+ */
+int timer_get_attributes(QEMUTimer *ts);
+
+/**
  * timer_deinit:
  * @ts: the timer to be de-initialised
  *
diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
index ca5cc3b..5c5a7f7 100644
--- a/tests/ptimer-test-stubs.c
+++ b/tests/ptimer-test-stubs.c
@@ -34,14 +34,15 @@ int64_t ptimer_test_time_ns;
 int use_icount = 1;
 bool qtest_allowed;
 
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque)
+void timer_init_a_tl(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
     ts->timer_list = timer_list;
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->attributes = attributes;
     ts->expire_time = -1;
 }
 
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index afb678f..d54ed09 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -31,7 +31,8 @@ static void co_sleep_cb(void *opaque)
     aio_co_wake(sleep_cb->co);
 }
 
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
+void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
+                                     int64_t ns)
 {
     AioContext *ctx = qemu_get_current_aio_context();
     CoSleepCB sleep_cb = {
@@ -46,7 +47,8 @@ void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
                 __func__, scheduled);
         abort();
     }
-    sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb);
+    sleep_cb.ts = aio_timer_new_a(ctx, type, SCALE_NS, attributes,
+                                  co_sleep_cb, &sleep_cb);
     timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
     timer_del(sleep_cb.ts);
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 86bfe84..29d8e39 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -339,17 +339,23 @@ int qemu_poll_ns(GPollFD *fds, guint nfds, int64_t timeout)
 }
 
 
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque)
+void timer_init_a_tl(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
     ts->timer_list = timer_list;
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->attributes = attributes;
     ts->expire_time = -1;
 }
 
+int timer_get_attributes(QEMUTimer *ts)
+{
+    return ts->attributes;
+}
+
 void timer_deinit(QEMUTimer *ts)
 {
     assert(ts->expire_time == -1);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 14/10/2018 16:55, Artem Pisarenko wrote:
> Attributes are simple flags, associated with individual timers for their whole lifetime.
> They intended to be used to mark individual timers for special handling by various qemu features operating at qemu core level.
> Existing timer, aio and coroutine interface extended with attribute-enabled variants of functions, which create/initialize timers.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Notes:
>     Conversion and association between QEMUTimerAttrBit and accessor macro are dumb.
>     Maybe better alternatives exist (like QFlags in Qt framework) or existing qemu code may be reused, if any.
>     Attributes also may be better named as flags, but they looks like something volatile, whereas 'attribute' expresses constant nature better.
> 
>  include/block/aio.h         |  50 +++++++++++++++++++-
>  include/qemu/coroutine.h    |   5 +-
>  include/qemu/timer.h        | 110 ++++++++++++++++++++++++++++++++++++++------
>  tests/ptimer-test-stubs.c   |   7 +--
>  util/qemu-coroutine-sleep.c |   6 ++-
>  util/qemu-timer.c           |  12 +++--
>  6 files changed, 165 insertions(+), 25 deletions(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..a6be3fb 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -407,10 +407,35 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, QEMUClockType type,
>                                         int scale,
>                                         QEMUTimerCB *cb, void *opaque)
>  {
> -    return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
> +    return timer_new_a_tl(ctx->tlg.tl[type], scale, 0, cb, opaque);
>  }

The new function _a_tl is a bit ugly.

I would prefer to:

- only have a new timer_new_full that takes all of scale, attribute and
timerlist

- accept a NULL timerlist and default to main_loop_tlg.tl[type]

- add aio_timer_{new,init}_with_attrs

> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f..cffc2b2 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -276,7 +276,10 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
>  /**
>   * Yield the coroutine for a given duration
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +#define qemu_co_sleep_ns(type, ns) \
> +    qemu_co_sleep_a_ns(type, 0, ns)
> +void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
> +                                     int64_t ns);

I wouldn't bother adding co_sleep_a_ns unless it's needed.

>  /**
>   * Yield until a file descriptor becomes readable
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 39ea907..031e3a1 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -52,6 +52,28 @@ typedef enum {
>      QEMU_CLOCK_MAX
>  } QEMUClockType;
>  
> +/**
> + * QEMU Timer attributes:
> + *
> + * An individual timer may be assigned with one or multiple attributes when
> + * initialized.
> + * Attribute is a static flag, meaning that timer has corresponding property.
> + * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
> + * which used to initialize timer, stored to 'attributes' member and can be
> + * retrieved externally with timer_get_attributes() call.
> + * Values of QEMUTimerAttrBit aren't used directly,
> + * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
> + * where 'id' is a unique part of attribute identifier.

I think QEMU_TIMER_ATTR is an unnecessary complication.  I would just
add a TIMER_ATTR_EXTERNAL constant.

Paolo

[Qemu-devel] [PATCH v2] Introduce attributes to qemu timer subsystem

[Artem Pisarenko]

Attributes are simple flags, associated with individual timers for their whole lifetime.
They intended to be used to mark individual timers for special handling by various qemu features operating at qemu core level.
New/init functions family in timer interface updated and their comments improved to avoid info duplication.
Also existing aio interface extended with attribute-enabled variants of functions, which create/initialize timers.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---

Notes:
    v2:
    - timer creation/initialize functions reworked and and their unnecessary variants removed (as Paolo Bonzini suggested)
    - also their comments improved to avoid info duplication

 include/block/aio.h       |  53 ++++++++++++++++++-
 include/qemu/timer.h      | 126 ++++++++++++++++++++++++++--------------------
 tests/ptimer-test-stubs.c |   7 +--
 util/qemu-timer.c         |  12 +++--
 4 files changed, 136 insertions(+), 62 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index f08630c..07c291a 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -407,10 +407,38 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, QEMUClockType type,
                                        int scale,
                                        QEMUTimerCB *cb, void *opaque)
 {
-    return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
+    return timer_new_full(type, ctx->tlg.tl[type],
+                          scale, 0, cb, opaque);
 }
 
 /**
+ * aio_timer_new_with_attrs:
+ * @ctx: the aio context
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Allocate a new timer (with attributes) attached to the context @ctx.
+ * The function is responsible for memory allocation.
+ *
+ * The preferred interface is aio_timer_init. Use that
+ * unless you really need dynamic memory allocation.
+ *
+ * Returns: a pointer to the new timer
+ */
+static inline QEMUTimer *aio_timer_new_with_attrs(AioContext *ctx,
+                                                  QEMUClockType type,
+                                                  int scale, int attributes,
+                                                  QEMUTimerCB *cb, void *opaque)
+{
+    return timer_new_full(type, ctx->tlg.tl[type],
+                          scale, attributes, cb, opaque);
+}
+
+
+/**
  * aio_timer_init:
  * @ctx: the aio context
  * @ts: the timer
@@ -427,7 +455,28 @@ static inline void aio_timer_init(AioContext *ctx,
                                   int scale,
                                   QEMUTimerCB *cb, void *opaque)
 {
-    timer_init_tl(ts, ctx->tlg.tl[type], scale, cb, opaque);
+    timer_init_full(ts, ctx->tlg.tl[type], scale, 0, cb, opaque);
+}
+
+/**
+ * aio_timer_init_with_attrs:
+ * @ctx: the aio context
+ * @ts: the timer
+ * @type: the clock type
+ * @scale: the scale
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
+ * @cb: the callback to call on timer expiry
+ * @opaque: the opaque pointer to pass to the callback
+ *
+ * Initialise a new timer (with attributes) attached to the context @ctx.
+ * The caller is responsible for memory allocation.
+ */
+static inline void aio_timer_init_with_attrs(AioContext *ctx,
+                                             QEMUTimer *ts, QEMUClockType type,
+                                             int scale, int attributes,
+                                             QEMUTimerCB *cb, void *opaque)
+{
+    timer_init_full(ts, ctx->tlg.tl[type], scale, attributes, cb, opaque);
 }
 
 /**
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 39ea907..64a84ea 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,6 +52,28 @@ typedef enum {
     QEMU_CLOCK_MAX
 } QEMUClockType;
 
+/**
+ * QEMU Timer attributes:
+ *
+ * An individual timer may be assigned with one or multiple attributes when
+ * initialized.
+ * Attribute is a static flag, meaning that timer has corresponding property.
+ * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
+ * which used to initialize timer, stored to 'attributes' member and can be
+ * retrieved externally with timer_get_attributes() call.
+ * Values of QEMUTimerAttrBit aren't used directly,
+ * instead each attribute in bit set accessed with QEMU_TIMER_ATTR_<id> macro,
+ * where <id> is a unique part of attribute identifier.
+ *
+ * No attributes defined currently.
+ */
+
+typedef enum {
+    QEMU_TIMER_ATTRBIT__NONE
+} QEMUTimerAttrBit;
+
+#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)
+
 typedef struct QEMUTimerList QEMUTimerList;
 
 struct QEMUTimerListGroup {
@@ -67,6 +89,7 @@ struct QEMUTimer {
     QEMUTimerCB *cb;
     void *opaque;
     QEMUTimer *next;
+    int attributes;
     int scale;
 };
 
@@ -418,22 +441,24 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup *tlg);
  */
 
 /**
- * timer_init_tl:
+ * timer_init_full:
  * @ts: the timer to be initialised
  * @timer_list: the timer list to attach the timer to
  * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
- * Initialise a new timer and associate it with @timer_list.
+ * Initialise a timer with the given scale and attributes,
+ * and associate it with @timer_list.
  * The caller is responsible for allocating the memory.
  *
  * You need not call an explicit deinit call. Simply make
  * sure it is not on a list with timer_del.
  */
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque);
+void timer_init_full(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque);
 
 /**
  * timer_init:
@@ -445,14 +470,12 @@ void timer_init_tl(QEMUTimer *ts,
  *
  * Initialize a timer with the given scale on the default timer list
  * associated with the clock.
- *
- * You need not call an explicit deinit call. Simply make
- * sure it is not on a list with timer_del.
+ * See timer_init_full for details.
  */
 static inline void timer_init(QEMUTimer *ts, QEMUClockType type, int scale,
                               QEMUTimerCB *cb, void *opaque)
 {
-    timer_init_tl(ts, main_loop_tlg.tl[type], scale, cb, opaque);
+    timer_init_full(ts, main_loop_tlg.tl[type], scale, 0, cb, opaque);
 }
 
 /**
@@ -464,9 +487,7 @@ static inline void timer_init(QEMUTimer *ts, QEMUClockType type, int scale,
  *
  * Initialize a timer with nanosecond scale on the default timer list
  * associated with the clock.
- *
- * You need not call an explicit deinit call. Simply make
- * sure it is not on a list with timer_del.
+ * See timer_init_full for details.
  */
 static inline void timer_init_ns(QEMUTimer *ts, QEMUClockType type,
                                  QEMUTimerCB *cb, void *opaque)
@@ -483,9 +504,7 @@ static inline void timer_init_ns(QEMUTimer *ts, QEMUClockType type,
  *
  * Initialize a timer with microsecond scale on the default timer list
  * associated with the clock.
- *
- * You need not call an explicit deinit call. Simply make
- * sure it is not on a list with timer_del.
+ * See timer_init_full for details.
  */
 static inline void timer_init_us(QEMUTimer *ts, QEMUClockType type,
                                  QEMUTimerCB *cb, void *opaque)
@@ -502,9 +521,7 @@ static inline void timer_init_us(QEMUTimer *ts, QEMUClockType type,
  *
  * Initialize a timer with millisecond scale on the default timer list
  * associated with the clock.
- *
- * You need not call an explicit deinit call. Simply make
- * sure it is not on a list with timer_del.
+ * See timer_init_full for details.
  */
 static inline void timer_init_ms(QEMUTimer *ts, QEMUClockType type,
                                  QEMUTimerCB *cb, void *opaque)
@@ -513,27 +530,40 @@ static inline void timer_init_ms(QEMUTimer *ts, QEMUClockType type,
 }
 
 /**
- * timer_new_tl:
- * @timer_list: the timer list to attach the timer to
+ * timer_new_full:
+ * @type: the clock type to use
+ * @timer_list: (optional) the timer list to attach the timer to
  * @scale: the scale value for the timer
+ * @attributes: 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values to assign
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
- * Create a new timer and associate it with @timer_list.
+ * Create a new timer with the given scale and attributes,
+ * and associate it with @timer_list or, if NULL, default timer list for
+ * the clock type @type.
  * The memory is allocated by the function.
  *
  * This is not the preferred interface unless you know you
- * are going to call timer_free. Use timer_init instead.
+ * are going to call timer_free. Use timer_init or timer_init_full instead.
+ *
+ * The default timer list has one special feature: in icount mode,
+ * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
+ * not true of other timer lists, which are typically associated
+ * with an AioContext---each of them runs its timer callbacks in its own
+ * AioContext thread.
  *
  * Returns: a pointer to the timer
  */
-static inline QEMUTimer *timer_new_tl(QEMUTimerList *timer_list,
-                                      int scale,
-                                      QEMUTimerCB *cb,
-                                      void *opaque)
+static inline QEMUTimer *timer_new_full(QEMUClockType type,
+                                        QEMUTimerList *timer_list,
+                                        int scale, int attributes,
+                                        QEMUTimerCB *cb, void *opaque)
 {
     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
-    timer_init_tl(ts, timer_list, scale, cb, opaque);
+    if (!timer_list) {
+        timer_list = main_loop_tlg.tl[type];
+    }
+    timer_init_full(ts, timer_list, scale, attributes, cb, opaque);
     return ts;
 }
 
@@ -544,21 +574,16 @@ static inline QEMUTimer *timer_new_tl(QEMUTimerList *timer_list,
  * @cb: the callback to be called when the timer expires
  * @opaque: the opaque pointer to be passed to the callback
  *
- * Create a new timer and associate it with the default
- * timer list for the clock type @type.
- *
- * The default timer list has one special feature: in icount mode,
- * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
- * not true of other timer lists, which are typically associated
- * with an AioContext---each of them runs its timer callbacks in its own
- * AioContext thread.
+ * Create a new timer with the given scale,
+ * and associate it with the default timer list for the clock type @type.
+ * See timer_new_full for details.
  *
  * Returns: a pointer to the timer
  */
 static inline QEMUTimer *timer_new(QEMUClockType type, int scale,
                                    QEMUTimerCB *cb, void *opaque)
 {
-    return timer_new_tl(main_loop_tlg.tl[type], scale, cb, opaque);
+    return timer_new_full(type, main_loop_tlg.tl[type], scale, 0, cb, opaque);
 }
 
 /**
@@ -569,12 +594,7 @@ static inline QEMUTimer *timer_new(QEMUClockType type, int scale,
  *
  * Create a new timer with nanosecond scale on the default timer list
  * associated with the clock.
- *
- * The default timer list has one special feature: in icount mode,
- * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
- * not true of other timer lists, which are typically associated
- * with an AioContext---each of them runs its timer callbacks in its own
- * AioContext thread.
+ * See timer_new_full for details.
  *
  * Returns: a pointer to the newly created timer
  */
@@ -590,14 +610,9 @@ static inline QEMUTimer *timer_new_ns(QEMUClockType type, QEMUTimerCB *cb,
  * @cb: the callback to call when the timer expires
  * @opaque: the opaque pointer to pass to the callback
  *
- * The default timer list has one special feature: in icount mode,
- * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
- * not true of other timer lists, which are typically associated
- * with an AioContext---each of them runs its timer callbacks in its own
- * AioContext thread.
- *
  * Create a new timer with microsecond scale on the default timer list
  * associated with the clock.
+ * See timer_new_full for details.
  *
  * Returns: a pointer to the newly created timer
  */
@@ -613,14 +628,9 @@ static inline QEMUTimer *timer_new_us(QEMUClockType type, QEMUTimerCB *cb,
  * @cb: the callback to call when the timer expires
  * @opaque: the opaque pointer to pass to the callback
  *
- * The default timer list has one special feature: in icount mode,
- * %QEMU_CLOCK_VIRTUAL timers are run in the vCPU thread.  This is
- * not true of other timer lists, which are typically associated
- * with an AioContext---each of them runs its timer callbacks in its own
- * AioContext thread.
- *
  * Create a new timer with millisecond scale on the default timer list
  * associated with the clock.
+ * See timer_new_full for details.
  *
  * Returns: a pointer to the newly created timer
  */
@@ -631,6 +641,14 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType type, QEMUTimerCB *cb,
 }
 
 /**
+ * timer_get_attributes:
+ * @ts: the timer
+ *
+ * Return 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values
+ */
+int timer_get_attributes(QEMUTimer *ts);
+
+/**
  * timer_deinit:
  * @ts: the timer to be de-initialised
  *
diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
index ca5cc3b..2b60216 100644
--- a/tests/ptimer-test-stubs.c
+++ b/tests/ptimer-test-stubs.c
@@ -34,14 +34,15 @@ int64_t ptimer_test_time_ns;
 int use_icount = 1;
 bool qtest_allowed;
 
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque)
+void timer_init_full(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
     ts->timer_list = timer_list;
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->attributes = attributes;
     ts->expire_time = -1;
 }
 
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 86bfe84..c579b05 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -339,17 +339,23 @@ int qemu_poll_ns(GPollFD *fds, guint nfds, int64_t timeout)
 }
 
 
-void timer_init_tl(QEMUTimer *ts,
-                   QEMUTimerList *timer_list, int scale,
-                   QEMUTimerCB *cb, void *opaque)
+void timer_init_full(QEMUTimer *ts,
+                     QEMUTimerList *timer_list, int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
     ts->timer_list = timer_list;
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->attributes = attributes;
     ts->expire_time = -1;
 }
 
+int timer_get_attributes(QEMUTimer *ts)
+{
+    return ts->attributes;
+}
+
 void timer_deinit(QEMUTimer *ts)
 {
     assert(ts->expire_time == -1);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 16/10/2018 15:03, Artem Pisarenko wrote:
> +    if (!timer_list) {
> +        timer_list = main_loop_tlg.tl[type];
> +    }
> +    timer_init_full(ts, timer_list, scale, attributes, cb, opaque);

Please move this "if" to timer_init_full, so that here you can just pass
timer_list.  timer_init_full will then take a QEMUTimerListGroup* (NULL
defaults to &main_loop_tlg, aio_timer_new passes &ctx->tlg) and a
QEMUClockType instead of a QEMUTimerList*.

Paolo

[Qemu-devel] [PATCH 3/3] Restores record/replay behavior related to special virtual clock processing for timers used in external subsystems.

[Artem Pisarenko]

Adds EXTERNAL attribute definition to qemu timers subsystem and assigns it to virtual clock timers, used in slirp (ICMP IPv6) and ui (key queue).
Virtual clock processing in rr mode reimplemented using this attribute.

Fixes: 87f4fe7653baf55b5c2f2753fe6003f473c07342
Fixes: 775a412bf83f6bc0c5c02091ee06cf649b34c593
Fixes: 9888091404a702d7ec79d51b088d994b9fc121bd
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---
 include/qemu/timer.h | 10 ++++++--
 slirp/ip6_icmp.c     |  4 +++-
 ui/input.c           |  5 ++--
 util/qemu-timer.c    | 67 ++++++++++++++++++++++++++++++++++++----------------
 4 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 031e3a1..53bfba5 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -65,11 +65,17 @@ typedef enum {
  * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
  * where 'id' is a unique part of attribute identifier.
  *
- * No attributes defined currently.
+ * The following attributes are available:
+ *
+ * QEMU_TIMER_ATTR(EXTERNAL): drives external subsystem
+ *
+ * Timers with this attribute do not recorded in rr mode, therefore it could be
+ * used for the subsystems that operate outside the guest core. Applicable only
+ * with virtual clock type.
  */
 
 typedef enum {
-    /* none */
+    QEMU_TIMER_ATTRBIT_EXTERNAL,
 } QEMUTimerAttrBit;
 
 #define QEMU_TIMER_ATTR(id) (1 << QEMU_TIMER_ATTRBIT_ ## id)
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index ee333d0..7c08433 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -27,7 +27,9 @@ void icmp6_init(Slirp *slirp)
         return;
     }
 
-    slirp->ra_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, ra_timer_handler, slirp);
+    slirp->ra_timer = timer_new_a(QEMU_CLOCK_VIRTUAL, SCALE_MS,
+                                  QEMU_TIMER_ATTR(EXTERNAL),
+                                  ra_timer_handler, slirp);
     timer_mod(slirp->ra_timer,
               qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + NDP_Interval);
 }
diff --git a/ui/input.c b/ui/input.c
index 51b1019..6279187 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -448,8 +448,9 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
     }
 
     if (!kbd_timer) {
-        kbd_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, qemu_input_queue_process,
-                                 &kbd_queue);
+        kbd_timer = timer_new_a(QEMU_CLOCK_VIRTUAL, SCALE_MS,
+                                QEMU_TIMER_ATTR(EXTERNAL),
+                                qemu_input_queue_process, &kbd_queue);
     }
     if (queue_count < queue_limit) {
         qemu_input_queue_delay(&kbd_queue, kbd_timer,
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 29d8e39..8c6d1cb 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     bool progress = false;
     QEMUTimerCB *cb;
     void *opaque;
+    bool need_replay_checkpoint = false;
 
     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -500,28 +501,52 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         goto out;
     }
 
-    switch (timer_list->clock->type) {
-    case QEMU_CLOCK_REALTIME:
-        break;
-    default:
-    case QEMU_CLOCK_VIRTUAL:
-        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
-            goto out;
-        }
-        break;
-    case QEMU_CLOCK_HOST:
-        if (!replay_checkpoint(CHECKPOINT_CLOCK_HOST)) {
-            goto out;
-        }
-        break;
-    case QEMU_CLOCK_VIRTUAL_RT:
-        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT)) {
-            goto out;
-        }
-        break;
-    }
-
     current_time = qemu_clock_get_ns(timer_list->clock->type);
+
+    if (replay_mode != REPLAY_MODE_NONE) {
+        switch (timer_list->clock->type) {
+        case QEMU_CLOCK_REALTIME:
+            break;
+        default:
+        case QEMU_CLOCK_VIRTUAL:
+            /* Check whether there are pending timers used for external
+             * subsystems, before doing replay checkpoint. If there are only
+             * such timers, then checkpoint will be redundant, because these
+             * timers don't change guest state directly.
+             * Procedure optimized to finish traversing timer list quickly,
+             * because it's a rare condition.
+             */
+            qemu_mutex_lock(&timer_list->active_timers_lock);
+            ts = timer_list->active_timers;
+            while (timer_expired_ns(ts, current_time)) {
+                if (!(ts->attributes & QEMU_TIMER_ATTR(EXTERNAL))) {
+                    need_replay_checkpoint = true;
+                    break;
+                }
+                ts = ts->next;
+            }
+            qemu_mutex_unlock(&timer_list->active_timers_lock);
+            if (!need_replay_checkpoint) {
+                break;
+            }
+
+            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
+                goto out;
+            }
+            break;
+        case QEMU_CLOCK_HOST:
+            if (!replay_checkpoint(CHECKPOINT_CLOCK_HOST)) {
+                goto out;
+            }
+            break;
+        case QEMU_CLOCK_VIRTUAL_RT:
+            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT)) {
+                goto out;
+            }
+            break;
+        }
+    }
+
     for(;;) {
         qemu_mutex_lock(&timer_list->active_timers_lock);
         ts = timer_list->active_timers;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] Restores record/replay behavior related to special virtual clock processing for timers used in external subsystems.

[Artem Pisarenko]

Opps, I introduced race condition in 'timerlist_run_timers' function. If
checking procedure decides, that no checkpoint needed, and another thread
adds expired non-EXTERNAL timer between end of checking procedure and end
of following "for (;;)" loop, then this timer will be processed but no
checkpoint will be preceeded. I didn't figured out how to workaround it
yet...

вс, 14 окт. 2018 г. в 20:57, Artem Pisarenko <artem.k.pisarenko@gmail.com>:

> Adds EXTERNAL attribute definition to qemu timers subsystem and assigns it
> to virtual clock timers, used in slirp (ICMP IPv6) and ui (key queue).
> Virtual clock processing in rr mode reimplemented using this attribute.
>
> Fixes: 87f4fe7653baf55b5c2f2753fe6003f473c07342
> Fixes: 775a412bf83f6bc0c5c02091ee06cf649b34c593
> Fixes: 9888091404a702d7ec79d51b088d994b9fc121bd
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
>  include/qemu/timer.h | 10 ++++++--
>  slirp/ip6_icmp.c     |  4 +++-
>  ui/input.c           |  5 ++--
>  util/qemu-timer.c    | 67
> ++++++++++++++++++++++++++++++++++++----------------
>  4 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 031e3a1..53bfba5 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -65,11 +65,17 @@ typedef enum {
>   * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id)
> macro,
>   * where 'id' is a unique part of attribute identifier.
>   *
> - * No attributes defined currently.
> + * The following attributes are available:
> + *
> + * QEMU_TIMER_ATTR(EXTERNAL): drives external subsystem
> + *
> + * Timers with this attribute do not recorded in rr mode, therefore it
> could be
> + * used for the subsystems that operate outside the guest core.
> Applicable only
> + * with virtual clock type.
>   */
>
>  typedef enum {
> -    /* none */
> +    QEMU_TIMER_ATTRBIT_EXTERNAL,
>  } QEMUTimerAttrBit;
>
>  #define QEMU_TIMER_ATTR(id) (1 << QEMU_TIMER_ATTRBIT_ ## id)
> diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
> index ee333d0..7c08433 100644
> --- a/slirp/ip6_icmp.c
> +++ b/slirp/ip6_icmp.c
> @@ -27,7 +27,9 @@ void icmp6_init(Slirp *slirp)
>          return;
>      }
>
> -    slirp->ra_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, ra_timer_handler,
> slirp);
> +    slirp->ra_timer = timer_new_a(QEMU_CLOCK_VIRTUAL, SCALE_MS,
> +                                  QEMU_TIMER_ATTR(EXTERNAL),
> +                                  ra_timer_handler, slirp);
>      timer_mod(slirp->ra_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + NDP_Interval);
>  }
> diff --git a/ui/input.c b/ui/input.c
> index 51b1019..6279187 100644
> --- a/ui/input.c
> +++ b/ui/input.c
> @@ -448,8 +448,9 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
>      }
>
>      if (!kbd_timer) {
> -        kbd_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> qemu_input_queue_process,
> -                                 &kbd_queue);
> +        kbd_timer = timer_new_a(QEMU_CLOCK_VIRTUAL, SCALE_MS,
> +                                QEMU_TIMER_ATTR(EXTERNAL),
> +                                qemu_input_queue_process, &kbd_queue);
>      }
>      if (queue_count < queue_limit) {
>          qemu_input_queue_delay(&kbd_queue, kbd_timer,
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index 29d8e39..8c6d1cb 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>      bool progress = false;
>      QEMUTimerCB *cb;
>      void *opaque;
> +    bool need_replay_checkpoint = false;
>
>      if (!atomic_read(&timer_list->active_timers)) {
>          return false;
> @@ -500,28 +501,52 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>          goto out;
>      }
>
> -    switch (timer_list->clock->type) {
> -    case QEMU_CLOCK_REALTIME:
> -        break;
> -    default:
> -    case QEMU_CLOCK_VIRTUAL:
> -        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
> -            goto out;
> -        }
> -        break;
> -    case QEMU_CLOCK_HOST:
> -        if (!replay_checkpoint(CHECKPOINT_CLOCK_HOST)) {
> -            goto out;
> -        }
> -        break;
> -    case QEMU_CLOCK_VIRTUAL_RT:
> -        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT)) {
> -            goto out;
> -        }
> -        break;
> -    }
> -
>      current_time = qemu_clock_get_ns(timer_list->clock->type);
> +
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        switch (timer_list->clock->type) {
> +        case QEMU_CLOCK_REALTIME:
> +            break;
> +        default:
> +        case QEMU_CLOCK_VIRTUAL:
> +            /* Check whether there are pending timers used for external
> +             * subsystems, before doing replay checkpoint. If there are
> only
> +             * such timers, then checkpoint will be redundant, because
> these
> +             * timers don't change guest state directly.
> +             * Procedure optimized to finish traversing timer list
> quickly,
> +             * because it's a rare condition.
> +             */
> +            qemu_mutex_lock(&timer_list->active_timers_lock);
> +            ts = timer_list->active_timers;
> +            while (timer_expired_ns(ts, current_time)) {
> +                if (!(ts->attributes & QEMU_TIMER_ATTR(EXTERNAL))) {
> +                    need_replay_checkpoint = true;
> +                    break;
> +                }
> +                ts = ts->next;
> +            }
> +            qemu_mutex_unlock(&timer_list->active_timers_lock);
> +            if (!need_replay_checkpoint) {
> +                break;
> +            }
> +
> +            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
> +                goto out;
> +            }
> +            break;
> +        case QEMU_CLOCK_HOST:
> +            if (!replay_checkpoint(CHECKPOINT_CLOCK_HOST)) {
> +                goto out;
> +            }
> +            break;
> +        case QEMU_CLOCK_VIRTUAL_RT:
> +            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT)) {
> +                goto out;
> +            }
> +            break;
> +        }
> +    }
> +
>      for(;;) {
>          qemu_mutex_lock(&timer_list->active_timers_lock);
>          ts = timer_list->active_timers;
> --
> 2.7.4
>
> --

С уважением,
  Артем Писаренко

Re: [Qemu-devel] [PATCH 3/3] Restores record/replay behavior related to special virtual clock processing for timers used in external subsystems.

[Paolo Bonzini]

On 14/10/2018 16:55, Artem Pisarenko wrote:
> +            qemu_mutex_lock(&timer_list->active_timers_lock);
> +            ts = timer_list->active_timers;
> +            while (timer_expired_ns(ts, current_time)) {
> +                if (!(ts->attributes & QEMU_TIMER_ATTR(EXTERNAL))) {
> +                    need_replay_checkpoint = true;
> +                    break;
> +                }
> +                ts = ts->next;
> +            }
> +            qemu_mutex_unlock(&timer_list->active_timers_lock);

This can be applied to all the timerlists, it doesn't have to be limited
to the "virtual" clock, something like (untested):

    qemu_mutex_lock(&timer_list->active_timers_lock);
    current_time = qemu_clock_get_ns(timer_list->clock->type);
    ts = timer_list->active_timers;
    if (replay_mode != REPLAY_MODE_NONE) {
         while (timer_expired_ns(ts, current_time)) {
             if (!(ts->attributes & QEMU_TIMER_ATTR(EXTERNAL))) {
                 qemu_mutex_unlock(&timer_list->active_timers_lock);
                 timerlist_checkpoint(timer_list);
                 qemu_mutex_lock(&timer_list->active_timers_lock);
                 break;
              }
              ts = ts->next;
         }
        ts = timer_list->active_timers;
    }

    while (timer_expired_ns(ts, current_time)) {
        /* remove timer from the list before calling the callback */
        timer_list->active_timers = ts->next;
        ts->next = NULL;
        ts->expire_time = -1;
        cb = ts->cb;
        opaque = ts->opaque;

        /* run the callback (the timer list can be modified) */
        qemu_mutex_unlock(&timer_list->active_timers_lock);
        cb(opaque);
        qemu_mutex_lock(&timer_list->active_timers_lock);
        progress = true;
        ts = timer_list->active_timers;
    }
    qemu_mutex_unlock(&timer_list->active_timers_lock);

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/3] Introduce attributes for timers subsystem and remove QEMU_CLOCK_VIRTUAL_EXT clock type

[Paolo Bonzini]

On 14/10/2018 16:55, Artem Pisarenko wrote:
> 
> Current implementation of timers/clock processing is confusing (at
> least for me) because of exceptions from design concept behind them,
> introduced by icount mode (which adds QEMU_CLOCK_VIRTUAL_RT). Adding
> QEMU_CLOCK_VIRTUAL_EXT just made things even more complicated. I
> consider these "alternative" virtual clocks to be some kind of hacks
> being convinient only to authors of relevant qemu features. Lets
> don't touch fundamental clock types and keep them orthogonal to
> special cases of timers handling.

VIRTUAL_RT is clear I believe, and it's used even beyond record/replay.
 However, I agree that VIRTUAL_EXT is messy and I like this series a lot.

Paolo