From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33369) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDNke-0001TU-AM for qemu-devel@nongnu.org; Fri, 19 Oct 2018 01:55:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gDNka-0008Jz-Rl for qemu-devel@nongnu.org; Fri, 19 Oct 2018 01:55:12 -0400 Received: from mail.ispras.ru ([83.149.199.45]:32904) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDNka-0007ow-CC for qemu-devel@nongnu.org; Fri, 19 Oct 2018 01:55:08 -0400 From: "Pavel Dovgalyuk" References: <3a396555c19468ce89788cc6deabbfef4ab56035.1539860473.git.artem.k.pisarenko@gmail.com> In-Reply-To: <3a396555c19468ce89788cc6deabbfef4ab56035.1539860473.git.artem.k.pisarenko@gmail.com> Date: Fri, 19 Oct 2018 08:55:09 +0300 Message-ID: <002201d46770$4affc060$e0ff4120$@ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Artem Pisarenko' , qemu-devel@nongnu.org Cc: 'Pavel Dovgalyuk' , 'Paolo Bonzini' > 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 > --- > > 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