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

[Qemu-devel] [PATCH v3 0/4] 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, which already 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 and handle as needed. This design achieves less dependencies, more transparency and has more intuitive and clear sense. For record/replay feature it's achieved with 'EXTERNAL' attribute. The only drawback is that it required to add one extra mutex unlock/lock pair for virtual clock type in timerlist_run_timers() function (see patch 3), but it's being added only when qemu runs in record/replay mode.

Finally, this patch series optimizes checkpointing for all other clocks it applies to.


v3 changes:
 - fixed failed build in last patch
 - attributes has been refactored and restricted to be used only within qemu-timer (as suggested by Stefan Hajnoczi and Paolo Bonzini)

v2 changes:
 - timers/aio interface refactored and improved, addition to couroutines removed (as Paolo Bonzini suggested)
 - fixed wrong reimplementation in qemu-timer.c caused race conditions
 - added bonus patch which optimizes checkpointing for other clocks

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.

P.S.2. I wasn't able to test these patches on purely clean master branch because of bugs https://bugs.launchpad.net/qemu/+bug/1790460 and https://bugs.launchpad.net/qemu/+bug/1795369, which workarounded by several not-accepted (yet?) modifications.


Artem Pisarenko (4):
  Revert some patches from recent [PATCH v6] "Fixing record/replay and
    adding reverse debugging"
  Introduce attributes to qemu timer subsystem
  Restores record/replay behavior related to special virtual clock
    processing for timers used in external subsystems.
  Optimize record/replay checkpointing for all clocks it applies to

 include/block/aio.h       |  59 ++++++++++++++++++---
 include/qemu/timer.h      | 128 +++++++++++++++++++++++-----------------------
 slirp/ip6_icmp.c          |   9 ++--
 tests/ptimer-test-stubs.c |  13 +++--
 ui/input.c                |   9 ++--
 util/qemu-timer.c         |  95 +++++++++++++++++++++++-----------
 6 files changed, 201 insertions(+), 112 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/4] Revert some patches from recent [PATCH v6] "Fixing record/replay and adding reverse debugging"

[Artem Pisarenko]

That patch series introduced new virtual clock type for use in external subsystems. It breaks desired behavior in non-record/replay usage scenarios.

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 v3 2/4] 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 which have integration into qemu-timer.
New/init functions family in timer interface updated and refactored (new 'attribute' argument added, timer_list replaced with timer_list_group+type combinations, 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:
    v3:
    - attributes has been properly incapsulated to qemu-timer (as suggested by Stefan Hajnoczi)
    - attributes definition and docs refactored to avoid extra enum and use simple macros with explicit bit positions (as suggested by Stefan Hajnoczi and Paolo Bonzini)
    - fixed old "QEMU_TIMER_ATTR(id)" notation (in comments) left from initial patch version
    
    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       |  59 ++++++++++++++++++++++---
 include/qemu/timer.h      | 110 +++++++++++++++++++++++-----------------------
 tests/ptimer-test-stubs.c |  13 ++++--
 util/qemu-timer.c         |  13 ++++--
 4 files changed, 125 insertions(+), 70 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index f08630c..0ca25df 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -388,6 +388,32 @@ struct LinuxAioState *aio_setup_linux_aio(AioContext *ctx, Error **errp);
 struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
 
 /**
+ * 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 or aio_timer_init_with_attrs.
+ * 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(&ctx->tlg, type, scale, attributes, cb, opaque);
+}
+
+/**
  * aio_timer_new:
  * @ctx: the aio context
  * @type: the clock type
@@ -396,10 +422,7 @@ struct LinuxAioState *aio_get_linux_aio(AioContext *ctx);
  * @opaque: the opaque pointer to pass to the callback
  *
  * Allocate a new timer 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.
+ * See aio_timer_new_with_attrs for details.
  *
  * Returns: a pointer to the new timer
  */
@@ -407,7 +430,29 @@ 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(&ctx->tlg, 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, type, scale, attributes, cb, opaque);
 }
 
 /**
@@ -420,14 +465,14 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, QEMUClockType type,
  * @opaque: the opaque pointer to pass to the callback
  *
  * Initialise a new timer attached to the context @ctx.
- * The caller is responsible for memory allocation.
+ * See aio_timer_init_with_attrs for details.
  */
 static inline void aio_timer_init(AioContext *ctx,
                                   QEMUTimer *ts, QEMUClockType type,
                                   int scale,
                                   QEMUTimerCB *cb, void *opaque)
 {
-    timer_init_tl(ts, ctx->tlg.tl[type], scale, cb, opaque);
+    timer_init_full(ts, &ctx->tlg, type, scale, 0, cb, opaque);
 }
 
 /**
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 39ea907..d45aded 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,6 +52,16 @@ typedef enum {
     QEMU_CLOCK_MAX
 } QEMUClockType;
 
+/**
+ * QEMU Timer attributes:
+ *
+ * An individual timer may be given one or multiple attributes when initialized.
+ * Each attribute corresponds to one bit. Attributes modify the processing
+ * of timers when they fire.
+ *
+ * No attributes defined currently.
+ */
+
 typedef struct QEMUTimerList QEMUTimerList;
 
 struct QEMUTimerListGroup {
@@ -67,6 +77,7 @@ struct QEMUTimer {
     QEMUTimerCB *cb;
     void *opaque;
     QEMUTimer *next;
+    int attributes;
     int scale;
 };
 
@@ -418,22 +429,28 @@ 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
+ * @timer_list_group: (optional) the timer list group to attach the timer to
+ * @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
  *
- * 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 for given clock @type in @timer_list_group
+ * (or default timer list group, if NULL).
  * 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,
+                     QEMUTimerListGroup *timer_list_group, QEMUClockType type,
+                     int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque);
 
 /**
  * timer_init:
@@ -445,14 +462,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, type, scale, 0, cb, opaque);
 }
 
 /**
@@ -464,9 +479,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 +496,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 +513,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 +522,38 @@ 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:
+ * @timer_list_group: (optional) the timer list group to attach the timer to
+ * @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 and associate it with @timer_list.
+ * Create a new timer with the given scale and attributes,
+ * and associate it with timer list for given clock @type in @timer_list_group
+ * (or default timer list group, if NULL).
  * 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(QEMUTimerListGroup *timer_list_group,
+                                        QEMUClockType type,
+                                        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_full(ts, timer_list_group, type, scale, attributes, cb, opaque);
     return ts;
 }
 
@@ -544,21 +564,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(&main_loop_tlg, type, scale, 0, cb, opaque);
 }
 
 /**
@@ -569,12 +584,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 +600,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 +618,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
  */
diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
index ca5cc3b..54b3fd2 100644
--- a/tests/ptimer-test-stubs.c
+++ b/tests/ptimer-test-stubs.c
@@ -34,14 +34,19 @@ 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,
+                     QEMUTimerListGroup *timer_list_group, QEMUClockType type,
+                     int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
-    ts->timer_list = timer_list;
+    if (!timer_list_group) {
+        timer_list_group = &main_loop_tlg;
+    }
+    ts->timer_list = timer_list_group->tl[type];
     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..04527a3 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -339,14 +339,19 @@ 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,
+                     QEMUTimerListGroup *timer_list_group, QEMUClockType type,
+                     int scale, int attributes,
+                     QEMUTimerCB *cb, void *opaque)
 {
-    ts->timer_list = timer_list;
+    if (!timer_list_group) {
+        timer_list_group = &main_loop_tlg;
+    }
+    ts->timer_list = timer_list_group->tl[type];
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->attributes = attributes;
     ts->expire_time = -1;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v3 3/4] 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 | 11 ++++++++++-
 slirp/ip6_icmp.c     |  4 +++-
 ui/input.c           |  5 +++--
 util/qemu-timer.c    | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index d45aded..dc0fd14 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -2,6 +2,7 @@
 #define QEMU_TIMER_H
 
 #include "qemu-common.h"
+#include "qemu/bitops.h"
 #include "qemu/notify.h"
 #include "qemu/host-utils.h"
 
@@ -59,9 +60,17 @@ typedef enum {
  * Each attribute corresponds to one bit. Attributes modify the processing
  * of timers when they fire.
  *
- * 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.
  */
 
+#define QEMU_TIMER_ATTR_EXTERNAL BIT(0)
+
 typedef struct QEMUTimerList QEMUTimerList;
 
 struct QEMUTimerListGroup {
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index ee333d0..cd1e0b9 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_full(NULL, 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..7c9a410 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_full(NULL, 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 04527a3..e2746cf 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -489,6 +489,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;
@@ -504,8 +505,15 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         break;
     default:
     case QEMU_CLOCK_VIRTUAL:
-        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
-            goto out;
+        if (replay_mode != REPLAY_MODE_NONE) {
+            /* Checkpoint for virtual clock is redundant in cases where
+             * it's being triggered with only non-EXTERNAL timers, because
+             * these timers don't change guest state directly.
+             * Since it has conditional dependence on specific timers, it is
+             * subject to race conditions and requires special handling.
+             * See below.
+             */
+            need_replay_checkpoint = true;
         }
         break;
     case QEMU_CLOCK_HOST:
@@ -520,13 +528,38 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         break;
     }
 
+    /*
+     * Extract expired timers from active timers list and and process them.
+     *
+     * In rr mode we need "filtered" checkpointing for virtual clock.
+     * Checkpoint must be replayed before any non-EXTERNAL timer has been
+     * processed and only one time (virtual clock value stays same). But these
+     * timers may appear in the timers list while it being processed, so this
+     * must be checked until we finally decide that "no timers left - we are
+     * done".
+     */
     current_time = qemu_clock_get_ns(timer_list->clock->type);
-    for(;;) {
-        qemu_mutex_lock(&timer_list->active_timers_lock);
-        ts = timer_list->active_timers;
+    qemu_mutex_lock(&timer_list->active_timers_lock);
+    while ((ts = timer_list->active_timers)) {
         if (!timer_expired_ns(ts, current_time)) {
+            /* No expired timers left.
+             * (If rr checkpoint was needed, it either already handled,
+             *  or may be skipped.) */
+            break;
+        }
+        if (need_replay_checkpoint
+                && !(ts->attributes & QEMU_TIMER_ATTR_EXTERNAL)) {
+            /* once we got here, checkpoint clock only once */
+            need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            break;
+            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
+                goto out;
+            }
+            qemu_mutex_lock(&timer_list->active_timers_lock);
+            /* it's better to start over again,
+             * just in case if timer list was modified
+             */
+            continue;
         }
 
         /* remove timer from the list before calling the callback */
@@ -535,12 +568,15 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         ts->expire_time = -1;
         cb = ts->cb;
         opaque = ts->opaque;
-        qemu_mutex_unlock(&timer_list->active_timers_lock);
 
         /* 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;
     }
+    qemu_mutex_unlock(&timer_list->active_timers_lock);
 
 out:
     qemu_event_set(&timer_list->timers_done_ev);
-- 
2.7.4

[Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
This also improves performance in rr mode.

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

Notes:
    v3:
    - fixed compiler warning caused non-debug build to fail

 include/qemu/timer.h |  2 +-
 util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index dc0fd14..bff8dac 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -65,7 +65,7 @@ typedef enum {
  * 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
+ * used for the subsystems that operate outside the guest core. Relevant only
  * with virtual clock type.
  */
 
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index e2746cf..216d107 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimerCB *cb;
     void *opaque;
     bool need_replay_checkpoint = false;
+    ReplayCheckpoint replay_checkpoint_id;
 
     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -500,43 +501,40 @@ 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_mode != REPLAY_MODE_NONE) {
-            /* Checkpoint for virtual clock is redundant in cases where
-             * it's being triggered with only non-EXTERNAL timers, because
-             * these timers don't change guest state directly.
-             * Since it has conditional dependence on specific timers, it is
-             * subject to race conditions and requires special handling.
-             * See below.
-             */
+    if (replay_mode != REPLAY_MODE_NONE) {
+        /* Postpone actual checkpointing to timer list processing
+         * to properly check if we actually need it.
+         */
+        switch (timer_list->clock->type) {
+        case QEMU_CLOCK_VIRTUAL:
             need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
+            break;
+        case QEMU_CLOCK_HOST:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
+            break;
+        case QEMU_CLOCK_VIRTUAL_RT:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
+            break;
+        default:
+            break;
         }
-        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;
     }
 
     /*
-     * Extract expired timers from active timers list and and process them.
+     * Extract expired timers from active timers list and and process them,
+     * taking into account checkpointing required in rr mode.
      *
-     * In rr mode we need "filtered" checkpointing for virtual clock.
-     * Checkpoint must be replayed before any non-EXTERNAL timer has been
-     * processed and only one time (virtual clock value stays same). But these
-     * timers may appear in the timers list while it being processed, so this
-     * must be checked until we finally decide that "no timers left - we are
-     * done".
+     * Checkpoint must be replayed before any timer has been processed
+     * and only one time. But new timers may appear in the timers list while
+     * it's being processed, so this must be checked until we finally decide
+     * that "no timers left - we are done" (to avoid skipping checkpoint due to
+     * possible races).
+     * Also checkpoint for virtual clock is redundant in cases where it's being
+     * triggered with only non-EXTERNAL timers, because these timers don't
+     * change guest state directly.
      */
     current_time = qemu_clock_get_ns(timer_list->clock->type);
     qemu_mutex_lock(&timer_list->active_timers_lock);
@@ -552,7 +550,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
             /* once we got here, checkpoint clock only once */
             need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
+            if (!replay_checkpoint(replay_checkpoint_id)) {
                 goto out;
             }
             qemu_mutex_lock(&timer_list->active_timers_lock);
-- 
2.7.4

[Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
This also improves performance in rr mode.

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

Oops, forgot to commit this fix

    v3:
    - fixed compiler warning caused non-debug build to fail

 include/qemu/timer.h |  2 +-
 util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index dc0fd14..bff8dac 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -65,7 +65,7 @@ typedef enum {
  * 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
+ * used for the subsystems that operate outside the guest core. Relevant only
  * with virtual clock type.
  */
 
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index e2746cf..47205fe 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimerCB *cb;
     void *opaque;
     bool need_replay_checkpoint = false;
+    ReplayCheckpoint replay_checkpoint_id = (ReplayCheckpoint)-1;
 
     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -500,43 +501,40 @@ 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_mode != REPLAY_MODE_NONE) {
-            /* Checkpoint for virtual clock is redundant in cases where
-             * it's being triggered with only non-EXTERNAL timers, because
-             * these timers don't change guest state directly.
-             * Since it has conditional dependence on specific timers, it is
-             * subject to race conditions and requires special handling.
-             * See below.
-             */
+    if (replay_mode != REPLAY_MODE_NONE) {
+        /* Postpone actual checkpointing to timer list processing
+         * to properly check if we actually need it.
+         */
+        switch (timer_list->clock->type) {
+        case QEMU_CLOCK_VIRTUAL:
             need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
+            break;
+        case QEMU_CLOCK_HOST:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
+            break;
+        case QEMU_CLOCK_VIRTUAL_RT:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
+            break;
+        default:
+            break;
         }
-        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;
     }
 
     /*
-     * Extract expired timers from active timers list and and process them.
+     * Extract expired timers from active timers list and and process them,
+     * taking into account checkpointing required in rr mode.
      *
-     * In rr mode we need "filtered" checkpointing for virtual clock.
-     * Checkpoint must be replayed before any non-EXTERNAL timer has been
-     * processed and only one time (virtual clock value stays same). But these
-     * timers may appear in the timers list while it being processed, so this
-     * must be checked until we finally decide that "no timers left - we are
-     * done".
+     * Checkpoint must be replayed before any timer has been processed
+     * and only one time. But new timers may appear in the timers list while
+     * it's being processed, so this must be checked until we finally decide
+     * that "no timers left - we are done" (to avoid skipping checkpoint due to
+     * possible races).
+     * Also checkpoint for virtual clock is redundant in cases where it's being
+     * triggered with only non-EXTERNAL timers, because these timers don't
+     * change guest state directly.
      */
     current_time = qemu_clock_get_ns(timer_list->clock->type);
     qemu_mutex_lock(&timer_list->active_timers_lock);
@@ -552,7 +550,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
             /* once we got here, checkpoint clock only once */
             need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
+            if (!replay_checkpoint(replay_checkpoint_id)) {
                 goto out;
             }
             qemu_mutex_lock(&timer_list->active_timers_lock);
-- 
2.7.4

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

[Artem Pisarenko]

Sorry, I forgot to fix long lines in commit messages. Do I need to submit
v4 ?

-- 

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

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

[Paolo Bonzini]

On 18/10/2018 14:00, Artem Pisarenko wrote:
> Sorry, I forgot to fix long lines in commit messages. Do I need to
> submit v4 ?
> 

No need, don't worry.

Paolo

[Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
This also improves performance in rr mode.

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

Oops and again oops. Now it finally should be fine...

    v3:
    - fixed compiler warning caused non-debug build to fail

 include/qemu/timer.h |  2 +-
 util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index dc0fd14..bff8dac 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -65,7 +65,7 @@ typedef enum {
  * 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
+ * used for the subsystems that operate outside the guest core. Relevant only
  * with virtual clock type.
  */
 
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index e2746cf..47205fe 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimerCB *cb;
     void *opaque;
     bool need_replay_checkpoint = false;
+    ReplayCheckpoint replay_checkpoint_id = (ReplayCheckpoint)-1;
 
     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -500,43 +501,40 @@ 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_mode != REPLAY_MODE_NONE) {
-            /* Checkpoint for virtual clock is redundant in cases where
-             * it's being triggered with only non-EXTERNAL timers, because
-             * these timers don't change guest state directly.
-             * Since it has conditional dependence on specific timers, it is
-             * subject to race conditions and requires special handling.
-             * See below.
-             */
+    if (replay_mode != REPLAY_MODE_NONE) {
+        /* Postpone actual checkpointing to timer list processing
+         * to properly check if we actually need it.
+         */
+        switch (timer_list->clock->type) {
+        case QEMU_CLOCK_VIRTUAL:
             need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
+            break;
+        case QEMU_CLOCK_HOST:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
+            break;
+        case QEMU_CLOCK_VIRTUAL_RT:
+            need_replay_checkpoint = true;
+            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
+            break;
+        default:
+            break;
         }
-        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;
     }
 
     /*
-     * Extract expired timers from active timers list and and process them.
+     * Extract expired timers from active timers list and and process them,
+     * taking into account checkpointing required in rr mode.
      *
-     * In rr mode we need "filtered" checkpointing for virtual clock.
-     * Checkpoint must be replayed before any non-EXTERNAL timer has been
-     * processed and only one time (virtual clock value stays same). But these
-     * timers may appear in the timers list while it being processed, so this
-     * must be checked until we finally decide that "no timers left - we are
-     * done".
+     * Checkpoint must be replayed before any timer has been processed
+     * and only one time. But new timers may appear in the timers list while
+     * it's being processed, so this must be checked until we finally decide
+     * that "no timers left - we are done" (to avoid skipping checkpoint due to
+     * possible races).
+     * Also checkpoint for virtual clock is redundant in cases where it's being
+     * triggered with only non-EXTERNAL timers, because these timers don't
+     * change guest state directly.
      */
     current_time = qemu_clock_get_ns(timer_list->clock->type);
     qemu_mutex_lock(&timer_list->active_timers_lock);
@@ -552,7 +550,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
             /* once we got here, checkpoint clock only once */
             need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
+            if (!replay_checkpoint(replay_checkpoint_id)) {
                 goto out;
             }
             qemu_mutex_lock(&timer_list->active_timers_lock);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Paolo Bonzini]

On 18/10/2018 13:16, Artem Pisarenko wrote:
> Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
> This also improves performance in rr mode.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Oops, forgot to commit this fix
> 
>     v3:
>     - fixed compiler warning caused non-debug build to fail

We can also move the switch statement to a separate function, it
simplifies the code:

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 8a2ad3bce2..3a64ce33d3 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -482,6 +482,26 @@ bool timer_expired(QEMUTimer *timer_head, int64_t
current_time)
     return timer_expired_ns(timer_head, current_time * timer_head->scale);
 }

+static bool timer_checkpoint(QEMUClockType clock)
+{
+    if (replay_mode != REPLAY_MODE_NONE) {
+        switch (clock) {
+        case QEMU_CLOCK_VIRTUAL:
+            return replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL);
+        case QEMU_CLOCK_HOST:
+            return replay_checkpoint(CHECKPOINT_CLOCK_HOST);
+        case QEMU_CLOCK_VIRTUAL_RT:
+            return replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT);
+        default:
+            /* QEMU_CLOCK_REALTIME is external to the emulation and does
+             * not need checkpointing.
+             */
+            break;
+        }
+    }
+    return true;
+}
+
 bool timerlist_run_timers(QEMUTimerList *timer_list)
 {
     QEMUTimer *ts;
@@ -489,8 +509,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     bool progress = false;
     QEMUTimerCB *cb;
     void *opaque;
-    bool need_replay_checkpoint = false;
-    ReplayCheckpoint replay_checkpoint_id;
+    bool need_replay_checkpoint = true;

     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -501,28 +520,6 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         goto out;
     }

-    if (replay_mode != REPLAY_MODE_NONE) {
-        /* Postpone actual checkpointing to timer list processing
-         * to properly check if we actually need it.
-         */
-        switch (timer_list->clock->type) {
-        case QEMU_CLOCK_VIRTUAL:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
-            break;
-        case QEMU_CLOCK_HOST:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
-            break;
-        case QEMU_CLOCK_VIRTUAL_RT:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
-            break;
-        default:
-            break;
-        }
-    }
-
     /*
      * Extract expired timers from active timers list and and process them,
      * taking into account checkpointing required in rr mode.
@@ -545,11 +542,11 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
             break;
         }
         if (need_replay_checkpoint
                 && !(ts->attributes & QEMU_TIMER_ATTR_EXTERNAL)) {
             /* once we got here, checkpoint clock only once */
             need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            if (!replay_checkpoint(replay_checkpoint_id)) {
+            if (!timer_checkpoint(timer_list->clock->type)) {
                 goto out;
             }
             qemu_mutex_lock(&timer_list->active_timers_lock);


No need to do anything on your part.

Paolo

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

> We can also move the switch statement to a separate function, it
> simplifies the code:
> ...

When I prepared this patch my intuition said me to add note in advance:
"Paolo, please, don't try to move this to a separate function. I've tried
it already. It cannot be done correct, look nice and not decrease
performancy at the same time". But I've ignored it... :)
Change you did is correct and nice, but (compared to my version) it adds
extra unlock/lock pair for running each timer list when it isn't empty and
in non-rr mode (where we would ignore checkpoints and execution flow would
bypass whole "if (need_replay_checkpoint) {...}" block).
Maybe you're aware of it, but I don't think that such change worth it.
-- 

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

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Paolo Bonzini]

On 18/10/2018 15:23, Artem Pisarenko wrote:
>> We can also move the switch statement to a separate function, it
>> simplifies the code:
>> ...
> 
> When I prepared this patch my intuition said me to add note in advance:
> "Paolo, please, don't try to move this to a separate function. I've
> tried it already. It cannot be done correct, look nice and not decrease
> performancy at the same time". But I've ignored it... :)
> Change you did is correct and nice, but (compared to my version) it adds
> extra unlock/lock pair for running each timer list when it isn't empty
> and in non-rr mode (where we would ignore checkpoints and execution flow
> would bypass whole "if (need_replay_checkpoint) {...}" block).
> Maybe you're aware of it, but I don't think that such change worth it.

No, you're right.  The if should remain in the caller, or
need_replay_checkpoint must be initialized with replay_mode.

Paolo

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

On Thu, Oct 18, 2018 at 05:04:29PM +0600, 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 which have integration into qemu-timer.
> New/init functions family in timer interface updated and refactored (new 'attribute' argument added, timer_list replaced with timer_list_group+type combinations, 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:
>     v3:
>     - attributes has been properly incapsulated to qemu-timer (as suggested by Stefan Hajnoczi)
>     - attributes definition and docs refactored to avoid extra enum and use simple macros with explicit bit positions (as suggested by Stefan Hajnoczi and Paolo Bonzini)
>     - fixed old "QEMU_TIMER_ATTR(id)" notation (in comments) left from initial patch version
>     
>     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       |  59 ++++++++++++++++++++++---
>  include/qemu/timer.h      | 110 +++++++++++++++++++++++-----------------------
>  tests/ptimer-test-stubs.c |  13 ++++--
>  util/qemu-timer.c         |  13 ++++--
>  4 files changed, 125 insertions(+), 70 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

> чт, 18 окт. 2018 г., 20:31 Paolo Bonzini:
>On 18/10/2018 15:23, Artem Pisarenko wrote:
>>> We can also move the switch statement to a separate function, it
>>> simplifies the code:
>>> ...
>>
>> When I prepared this patch my intuition said me to add note in advance:
>> "Paolo, please, don't try to move this to a separate function. I've
>> tried it already. It cannot be done correct, look nice and not decrease
>> performancy at the same time". But I've ignored it... :)
>> Change you did is correct and nice, but (compared to my version) it adds
>> extra unlock/lock pair for running each timer list when it isn't empty
>> and in non-rr mode (where we would ignore checkpoints and execution flow
>> would bypass whole "if (need_replay_checkpoint) {...}" block).
>> Maybe you're aware of it, but I don't think that such change worth it.
>
> No, you're right. The if should remain in the caller, or
> need_replay_checkpoint must be initialized with replay_mode.

If initialize 'need_replay_checkpoint', then it should also account for
clock != QEMU_CLOCK_REALTIME. And here we come to what if+switch block
actually (mostly) does in my version. Finally, you will get duplication of
this whole condition usage between source function and extracted function,
which isn't nice.
Why do you want to split up such tightly coupled code?

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Paolo Bonzini]

On 18/10/2018 19:10, Artem Pisarenko wrote:
> 
>> No, you're right. The if should remain in the caller, or
>> need_replay_checkpoint must be initialized with replay_mode.
> 
> If initialize 'need_replay_checkpoint', then it should also account for
> clock != QEMU_CLOCK_REALTIME.

Or you just get a unlock/lock pair for QEMU_CLOCK_REALTIME (which should
really never happen if e.g. you have no UI).

> And here we come to what if+switch block
> actually (mostly) does in my version. Finally, you will get duplication
> of this whole condition usage between source function and extracted
> function, which isn't nice.
> Why do you want to split up such tightly coupled code?

Because it's *too* coupled and not very readable.

Paolo

Re: [Qemu-devel] [PATCH v3] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

>чт, 18 окт. 2018 г., 23:25 Paolo Bonzini:
>On 18/10/2018 19:10, Artem Pisarenko wrote:
>>
>>> No, you're right. The if should remain in the caller, or
>>> need_replay_checkpoint must be initialized with replay_mode.
>>
>> If initialize 'need_replay_checkpoint', then it should also account for
>> clock != QEMU_CLOCK_REALTIME.
>
> Or you just get a unlock/lock pair for QEMU_CLOCK_REALTIME (which should
> really never happen if e.g. you have no UI).

And still have duplication (just smaller in this case).


>> Why do you want to split up such tightly coupled code?
>
> Because it's *too* coupled and not very readable.

Tightly coupled code in my understanding is property having its roots in
design, which usually has wider context than piece of code in question. So
we canot avoid this by definition (limiting changes only to this code).
Trying to improve it is just like a playing with bubble wrap - pressing
each bubble causes another bubble to pop up.

In our particular case it's because of overall rr design, qemu architecture
and a way how rr integrated in timers. The best we can do in this case is
to localise/quarantine ugly aspects as much as possible, carefully and
plenteously comment and try don't touch them... never... I consider
'timerlist_run_timers()' is already totally infected and we're just late to
save anyone of its residents (by isolating others).

Re: [Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

[Pavel Dovgalyuk]

> From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> Removes redundant checkpoints in replay log when there are no expired timers in timers list,
> associated with corresponding clock (i.e. no rr events associated with current clock value).
> This also improves performance in rr mode.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Notes:
>     v3:
>     - fixed compiler warning caused non-debug build to fail
> 
>  include/qemu/timer.h |  2 +-
>  util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
>  2 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index dc0fd14..bff8dac 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -65,7 +65,7 @@ typedef enum {
>   * 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
> + * used for the subsystems that operate outside the guest core. Relevant only
>   * with virtual clock type.
>   */
> 
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index e2746cf..216d107 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>      QEMUTimerCB *cb;
>      void *opaque;
>      bool need_replay_checkpoint = false;
> +    ReplayCheckpoint replay_checkpoint_id;
> 
>      if (!atomic_read(&timer_list->active_timers)) {
>          return false;
> @@ -500,43 +501,40 @@ 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_mode != REPLAY_MODE_NONE) {
> -            /* Checkpoint for virtual clock is redundant in cases where
> -             * it's being triggered with only non-EXTERNAL timers, because
> -             * these timers don't change guest state directly.
> -             * Since it has conditional dependence on specific timers, it is
> -             * subject to race conditions and requires special handling.
> -             * See below.
> -             */
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        /* Postpone actual checkpointing to timer list processing
> +         * to properly check if we actually need it.
> +         */
> +        switch (timer_list->clock->type) {
> +        case QEMU_CLOCK_VIRTUAL:
>              need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
> +            break;
> +        case QEMU_CLOCK_HOST:
> +            need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
> +            break;

This is wrong at least for QEMU_CLOCK_HOST.

> +        case QEMU_CLOCK_VIRTUAL_RT:
> +            need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
> +            break;
> +        default:
> +            break;
>          }
> -        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;
>      }
> 
>      /*
> -     * Extract expired timers from active timers list and and process them.
> +     * Extract expired timers from active timers list and and process them,
> +     * taking into account checkpointing required in rr mode.
>       *
> -     * In rr mode we need "filtered" checkpointing for virtual clock.
> -     * Checkpoint must be replayed before any non-EXTERNAL timer has been
> -     * processed and only one time (virtual clock value stays same). But these
> -     * timers may appear in the timers list while it being processed, so this
> -     * must be checked until we finally decide that "no timers left - we are
> -     * done".
> +     * Checkpoint must be replayed before any timer has been processed
> +     * and only one time. But new timers may appear in the timers list while
> +     * it's being processed, so this must be checked until we finally decide
> +     * that "no timers left - we are done" (to avoid skipping checkpoint due to
> +     * possible races).
> +     * Also checkpoint for virtual clock is redundant in cases where it's being
> +     * triggered with only non-EXTERNAL timers, because these timers don't
> +     * change guest state directly.
>       */
>      current_time = qemu_clock_get_ns(timer_list->clock->type);

Reading the host clock here is not protected by the checkpoint.
Therefore it may incur the inconsistency when replaying the execution.

>      qemu_mutex_lock(&timer_list->active_timers_lock);
> @@ -552,7 +550,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>              /* once we got here, checkpoint clock only once */
>              need_replay_checkpoint = false;
>              qemu_mutex_unlock(&timer_list->active_timers_lock);
> -            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
> +            if (!replay_checkpoint(replay_checkpoint_id)) {
>                  goto out;
>              }
>              qemu_mutex_lock(&timer_list->active_timers_lock);
> --
> 2.7.4


Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

[Artem Pisarenko]

> …
> This is wrong at least for QEMU_CLOCK_HOST.
> …
> Reading the host clock here is not protected by the checkpoint.
> Therefore it may incur the inconsistency when replaying the execution.

That's why I didn't like idea of this patch and asked for any possible side
effects beforehand. So, here is one.
I have no idea how to fix it properly and would like to recall this
particular patch from these series (remember, it doesn't match series
goal). Lets leave this patch/issue for separate resolution, although I'm
not very interested in it.

Re: [Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

[Paolo Bonzini]

On 19/10/2018 08:30, Artem Pisarenko wrote:
>> This is wrong at least for QEMU_CLOCK_HOST.
>> …
>> Reading the host clock here is not protected by the checkpoint.
>> Therefore it may incur the inconsistency when replaying the execution.
> 
> That's why I didn't like idea of this patch and asked for any possible
> side effects beforehand. So, here is one.
> I have no idea how to fix it properly and would like to recall this
> particular patch from these series (remember, it doesn't match series
> goal). Lets leave this patch/issue for separate resolution, although I'm
> not very interested in it.

Yup, I'll send v2 of the pull request with this patch removed.

Paolo

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

[Pavel Dovgalyuk]

Hi,

It seems, that this approach is not always correct.
Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including external
ones).
qemu_start_warp_timer uses the deadline for setting warp timer (which should be deterministic).
Therefore warp timer may become non-deterministic and replay may behave differently compared to
recording phase.

We have to rollback these or improve somehow to avoid non-determinism.

Pavel Dovgalyuk

> -----Original Message-----
> From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> Sent: Thursday, October 18, 2018 2:04 PM
> To: qemu-devel@nongnu.org
> Cc: Pavel Dovgalyuk; Paolo Bonzini; Artem Pisarenko
> Subject: [PATCH v3 0/4] Introduce attributes for timers subsystem and remove
> QEMU_CLOCK_VIRTUAL_EXT clock type
> 
> 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, which already 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 and handle as needed. This design achieves less
> dependencies, more transparency and has more intuitive and clear sense. For record/replay
> feature it's achieved with 'EXTERNAL' attribute. The only drawback is that it required to add
> one extra mutex unlock/lock pair for virtual clock type in timerlist_run_timers() function
> (see patch 3), but it's being added only when qemu runs in record/replay mode.
> 
> Finally, this patch series optimizes checkpointing for all other clocks it applies to.
> 
> 
> v3 changes:
>  - fixed failed build in last patch
>  - attributes has been refactored and restricted to be used only within qemu-timer (as
> suggested by Stefan Hajnoczi and Paolo Bonzini)
> 
> v2 changes:
>  - timers/aio interface refactored and improved, addition to couroutines removed (as Paolo
> Bonzini suggested)
>  - fixed wrong reimplementation in qemu-timer.c caused race conditions
>  - added bonus patch which optimizes checkpointing for other clocks
> 
> 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.
> 
> P.S.2. I wasn't able to test these patches on purely clean master branch because of bugs
> https://bugs.launchpad.net/qemu/+bug/1790460 and https://bugs.launchpad.net/qemu/+bug/1795369,
> which workarounded by several not-accepted (yet?) modifications.
> 
> 
> Artem Pisarenko (4):
>   Revert some patches from recent [PATCH v6] "Fixing record/replay and
>     adding reverse debugging"
>   Introduce attributes to qemu timer subsystem
>   Restores record/replay behavior related to special virtual clock
>     processing for timers used in external subsystems.
>   Optimize record/replay checkpointing for all clocks it applies to
> 
>  include/block/aio.h       |  59 ++++++++++++++++++---
>  include/qemu/timer.h      | 128 +++++++++++++++++++++++-----------------------
>  slirp/ip6_icmp.c          |   9 ++--
>  tests/ptimer-test-stubs.c |  13 +++--
>  ui/input.c                |   9 ++--
>  util/qemu-timer.c         |  95 +++++++++++++++++++++++-----------
>  6 files changed, 201 insertions(+), 112 deletions(-)
> 
> --
> 2.7.4

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

[Paolo Bonzini]

On 10/01/19 14:30, Pavel Dovgalyuk wrote:
> Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including external
> ones).
> qemu_start_warp_timer uses the deadline for setting warp timer (which should be deterministic).

Can you introduce a variant of qemu_clock_deadline_ns_all that ignores
external timers, and use it in qemu_start_warp_timer?

timerlist_deadline_ns can be made static, so adding a bool argument to
it is not a big issue for API cleanliness.

In fact, qemu_clock_deadline_ns_all is only ever used with
QEMU_CLOCK_VIRTUAL, so you could also change it to

uint64_t virtual_clock_deadline_ns(bool include_external);

Paolo

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

[Artem Pisarenko]

> It seems, that this approach is not always correct.
> Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including external
> ones).
> qemu_start_warp_timer uses the deadline for setting warp timer (which should be deterministic).
> Therefore warp timer may become non-deterministic and replay may behave differently compared to
> recording phase.
>
> We have to rollback these or improve somehow to avoid non-determinism.

I dont understand how this approach would even introduce
non-determinism. I'm not sure about aspects of timers subsystem you
mentioned, assuming that maybe we missed something when tried to
optimize. But this has nothing to do with determinism as long as we
treat all virtual clock timers as deterministic, regardless of
EXTERNAL attribute being set or not. They are intended to be used as
such by design, aren't? This attribute was introduced purely to avoid
extra events in log.

Artem

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

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 10/01/19 14:30, Pavel Dovgalyuk wrote:
> > Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including
> external
> > ones).
> > qemu_start_warp_timer uses the deadline for setting warp timer (which should be
> deterministic).
> 
> Can you introduce a variant of qemu_clock_deadline_ns_all that ignores
> external timers, and use it in qemu_start_warp_timer?

Ok.

> timerlist_deadline_ns can be made static, so adding a bool argument to
> it is not a big issue for API cleanliness.
> 
> In fact, qemu_clock_deadline_ns_all is only ever used with
> QEMU_CLOCK_VIRTUAL, so you could also change it to
> 
> uint64_t virtual_clock_deadline_ns(bool include_external);

I started making changes and it seems that this parameter should never be true,
because this function is called either from cpus.c or from the tests.

Pavel Dovgalyuk

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

[Pavel Dovgalyuk]

> From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> > It seems, that this approach is not always correct.
> > Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including
> external
> > ones).
> > qemu_start_warp_timer uses the deadline for setting warp timer (which should be
> deterministic).
> > Therefore warp timer may become non-deterministic and replay may behave differently compared
> to
> > recording phase.
> >
> > We have to rollback these or improve somehow to avoid non-determinism.
> 
> I dont understand how this approach would even introduce
> non-determinism. I'm not sure about aspects of timers subsystem you
> mentioned, assuming that maybe we missed something when tried to
> optimize. But this has nothing to do with determinism as long as we
> treat all virtual clock timers as deterministic, regardless of
> EXTERNAL attribute being set or not. They are intended to be used as
> such by design, aren't? This attribute was introduced purely to avoid
> extra events in log.

Right, but deadline calculation and icount warp events (that are written into the log too)
depend on the active virtual timers. Therefore external ones may affect icount warp
operation sequence.

Pavel Dovgalyuk

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

[Artem Pisarenko]

ср, 16 янв. 2019 г. в 12:15, Pavel Dovgalyuk <dovgaluk@ispras.ru>:
>
> > From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> > > It seems, that this approach is not always correct.
> > > Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including
> > external
> > > ones).
> > > qemu_start_warp_timer uses the deadline for setting warp timer (which should be
> > deterministic).
> > > Therefore warp timer may become non-deterministic and replay may behave differently compared
> > to
> > > recording phase.
> > >
> > > We have to rollback these or improve somehow to avoid non-determinism.
> >
> > I dont understand how this approach would even introduce
> > non-determinism. I'm not sure about aspects of timers subsystem you
> > mentioned, assuming that maybe we missed something when tried to
> > optimize. But this has nothing to do with determinism as long as we
> > treat all virtual clock timers as deterministic, regardless of
> > EXTERNAL attribute being set or not. They are intended to be used as
> > such by design, aren't? This attribute was introduced purely to avoid
> > extra events in log.
>
> Right, but deadline calculation and icount warp events (that are written into the log too)
> depend on the active virtual timers. Therefore external ones may affect icount warp
> operation sequence.
>
> Pavel Dovgalyuk
>

Sorry, but I still don't understand the source of non-determinism.
>From what you said I may understand that replay module actually has
some additional non-trivial (for me) dependencies on EXTERNAL
attribute other than one, introduced in 'timerlist_run_timers()' (i.e.
it expects deadline calculations somewhere else to match decisions
made in this function, or something like that). I would agree that
these patch series might introduce some incorrect behavior. But it
must be identical in all emulations running in same conditions,
because deadline, warp timer and their effects are all dependent only
on virtual timers, and, therefore, are deterministic too. Of course,
it needs to be fixed. Did you find solution ?

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

[Pavel Dovgalyuk]

> From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> ср, 16 янв. 2019 г. в 12:15, Pavel Dovgalyuk <dovgaluk@ispras.ru>:
> >
> > > From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> > > > It seems, that this approach is not always correct.
> > > > Now timerlist_deadline_ns uses all virtual timers for deadline calculation (including
> > > external
> > > > ones).
> > > > qemu_start_warp_timer uses the deadline for setting warp timer (which should be
> > > deterministic).
> > > > Therefore warp timer may become non-deterministic and replay may behave differently
> compared
> > > to
> > > > recording phase.
> > > >
> > > > We have to rollback these or improve somehow to avoid non-determinism.
> > >
> > > I dont understand how this approach would even introduce
> > > non-determinism. I'm not sure about aspects of timers subsystem you
> > > mentioned, assuming that maybe we missed something when tried to
> > > optimize. But this has nothing to do with determinism as long as we
> > > treat all virtual clock timers as deterministic, regardless of
> > > EXTERNAL attribute being set or not. They are intended to be used as
> > > such by design, aren't? This attribute was introduced purely to avoid
> > > extra events in log.
> >
> > Right, but deadline calculation and icount warp events (that are written into the log too)
> > depend on the active virtual timers. Therefore external ones may affect icount warp
> > operation sequence.
> >
> > Pavel Dovgalyuk
> >
> 
> Sorry, but I still don't understand the source of non-determinism.

The source is the external timers, that depend on "outer world", e.g., slirp.
These timers are not recorded/replayed, but may affect the icount warping
(which should remain deterministic).

> From what you said I may understand that replay module actually has
> some additional non-trivial (for me) dependencies on EXTERNAL
> attribute other than one, introduced in 'timerlist_run_timers()' (i.e.
> it expects deadline calculations somewhere else to match decisions
> made in this function, or something like that). I would agree that
> these patch series might introduce some incorrect behavior. But it
> must be identical in all emulations running in same conditions,
> because deadline, warp timer and their effects are all dependent only
> on virtual timers, and, therefore, are deterministic too. Of course,
> it needs to be fixed. Did you find solution ?

Please check the new version of the series.
Patch 22

Pavel Dovgalyuk

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

[Artem Pisarenko]

> The source is the external timers, that depend on "outer world", e.g., slirp.
> These timers are not recorded/replayed, but may affect the icount warping
> (which should remain deterministic).

I thought we agreed earlier in this discussion that external timers
are deterministic by design, because they are subset of virtual clock
timers. Regarding slirp module, although it interfaces "outer world"
(except when 'restrict' option is on), it still uses IPv6 timer
deterministically. You may check it yourself. Otherwise it shouldn't
use timer of 'virtual clock' type for this purpose and this needs to
be fixed.
So I still don't understand. And I'm not asking you to explain. I'm
just hinting you that your fix may finally happen to be incorrect in
case if it based on wrong assertion I'm talking about.

> Please check the new version of the series.

Sorry, I'm too busy right now for deep analysis. I've just tried to
briefly understand issue you pointed and provide minimal feedback in
order to prevent possible wrong decisions. I'll return to this work
later.