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

[Steven Sistare <steven.sistare@oracle.com>]

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) {