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.pisare...@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.pisare...@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
>
> --
С уважением,
Артем Писаренко
