| Message ID | ecc524bb47e169394f9bed88c979fdc8d990111a.1539861171.git.artem.k.pisarenko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Optimize record/replay checkpointing for all clocks it applies to | expand |
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
> 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.
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
> чт, 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?
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
>чт, 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).
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);
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(-)