diff mbox series

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

Message ID f203d1b772f7e4087f8e64f5f23ae313835413d4.1539528213.git.artem.k.pisarenko@gmail.com
State New
Headers show
Series Introduce attributes for timers subsystem and remove QEMU_CLOCK_VIRTUAL_EXT clock type | expand

Commit Message

Artem Pisarenko Oct. 14, 2018, 2:55 p.m. UTC 
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(-)

Comments

Artem Pisarenko Oct. 15, 2018, 6:41 a.m. UTC | #1 
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
>
> --

С уважением,
  Артем Писаренко
Paolo Bonzini Oct. 15, 2018, 8:51 a.m. UTC | #2 
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
diff mbox series

Patch

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;