Re: [RFC V1 2/7] migration: skip dirty memory tracking for cpr

[Steven Sistare]

On 8/12/2024 2:57 PM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
>
> > CPR preserves memory in place, so there is no need to track dirty memory.
> > By skipping it, CPR can support devices that do not support tracking.
> >
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > ---
> >   system/memory.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/system/memory.c b/system/memory.c
> > index b7548bf112..aef584e638 100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -27,6 +27,7 @@
> >   
> >   #include "exec/memory-internal.h"
> >   #include "exec/ram_addr.h"
> > +#include "migration/misc.h"
> >   #include "sysemu/kvm.h"
> >   #include "sysemu/runstate.h"
> >   #include "sysemu/tcg.h"
> > @@ -2947,6 +2948,11 @@ bool memory_global_dirty_log_start(unsigned int flags, 
> > Error **errp)
> >   
> >       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> >   
> > +    /* CPR preserves memory in place, so no need to track dirty memory */
> > +    if (migrate_mode() != MIG_MODE_NORMAL) {
> > +        return true;
> > +    }
>
> How this interacts with DIRTY_RATE and DIRTY_LIMIT? The former at least
> seems to never overlap with CPR, right? I'm wondering whether this check
> would be more appropriate up in ram.c along with the similar
> migrate_background_snapshot() check.
Agreed.  I previously pushed the CPR check down to memory_global_dirty_log_start
to catch all callers, but the only callers that matter are ram_init_bitmaps and
ram_save_cleanup, so I will hoist the check to those callers.

> (I wish we had made the global_dirty_log_change() function a bit more
> flexible. It would have been a nice place to put this and the snapshot
> check. Not worth the risk of changing it now...)
>
> Also, not tracking dirty memory implies also not doing the bitmap sync?
> We skip it for bg_snapshot, but not for CPR.

Good catch, I must skip it for CPR also:

--------------------------
@@ -3250,7 +3261,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     rs->last_stage = !migration_in_colo_state();

     WITH_RCU_READ_LOCK_GUARD() {
-        if (!migration_in_postcopy()) {
+        /* We don't use dirty log with CPR. */
+        if (!migration_in_postcopy() && migrate_mode() == MIG_MODE_NORMAL) {
             migration_bitmap_sync_precopy(rs, true);
         }
---------------------------

- Steve

> > +
> >       if (vmstate_change) {
> >           /* If there is postponed stop(), operate on it first */
> >           postponed_stop_flags &= ~flags;
> > @@ -3021,6 +3027,11 @@ static void memory_vm_change_state_handler(void *opaque, 
> > bool running,
> >   
> >   void memory_global_dirty_log_stop(unsigned int flags)
> >   {
> > +    /* CPR preserves memory in place, so no need to track dirty memory */
> > +    if (migrate_mode() != MIG_MODE_NORMAL) {
> > +        return;
> > +    }
> > +
> >       if (!runstate_is_running()) {
> >           /* Postpone the dirty log stop, e.g., to when VM starts again */
> >           if (vmstate_change) {