We better double check and document that, because it must be guaranteed, not
"let's cross fingers".
Yes, we should double check on at least known good use cases, maybe not
all.
E.g., I see nvmm_log_sync() and whpx_log_sync() unconditionally set dirty
to all mem always. I actually don't know how some of these trackers work
for live migration, or if it works at all.
If by accident this change breaks something, I also wonder whether we
should fix the patch that breaks it, or fix the assumption that the 1st
sync must happen at setup. That's simply wrong to me.. and not all dirty
track users have such either. E.g. vga tracking won't even have a SETUP
phase at all.
The simplest solution for any potential breakage is to provide a
log_global_start() and sync once there, that's exactly what SETUP should
do. But I hope it won't happen at all..
This is legacy tech debt to me. The earlier we understand it the better,
so I'm personally ok if someone would try to do this as long as major archs
will be safe.
Also, I'm not 100% sure how KVM internals implement that (I recall some
s390x oddities, but might be wrong ...).
virtio-balloon() calls qemu_guest_free_page_hint() which calls
migration_clear_memory_region_dirty_bitmap_range()
bitmap_clear()
So it would maybe have the same issue.
Should virtio-balloon do the same?
virtio-balloon is more interesting, because I assume here we could run after
the "initial clearing" and would want to mark it clean everywhere.
Yes, if it does what I mentioned below, IIUC it'll clear all dirty bits
across the whole stack. Only the ram_list bitmap is missing. IIUC it
could mean it could stop working for at least tcg, as tcg sololy uses
it.. even with kvm some MRs may use it. Maybe we want to fix it
separately.
Yes, that definitely needs care.
[...]
(but I'm confused because we have way too many bitmaps, and maybe the KVM
one could be problematic without an initial sync ... we'd want an initial
clearing for that as well ...)
So IMHO most of the bitmaps should be initialized with zeros, not
ones.. like I mentioned above.
Migration bitmap is special, because it's not about dirty tracking
capability / reporting but that we know we need to migrate the first round.
So setting all ones makes sense for migration only, not a reporting
facility. While KVM_DIRTY_LOG_INITIALLY_SET existed for its own reasoning
on speeding up migration starts..
So, now I am thinking whether we should not set all ones in ram_list bitmap
at all, corresponds to this change:
===8<===
diff --git a/system/physmem.c b/system/physmem.c
index dc1db3a384..10966fa68c 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1913,10 +1913,6 @@ static void ram_block_add(RAMBlock *new_block, Error
**errp)
ram_list.version++;
qemu_mutex_unlock_ramlist();
- cpu_physical_memory_set_dirty_range(new_block->offset,
- new_block->used_length,
- DIRTY_CLIENTS_ALL);
-
if (new_block->host) {
qemu_ram_setup_dump(new_block->host, new_block->max_length);
qemu_madvise(new_block->host, new_block->max_length,
QEMU_MADV_HUGEPAGE);
===8<===
It will leave things in an inconsistent state with regards of
qemu_ram_resize() though. So we'd want to do the same thing there as well.
I don't know about DIRTY_CLIENTS_ALL, though ... no expert on that, which
side effects it has.
Because saying "initially, all memory is dirty" can make sense, depending on
from which angle you look at it.
Migration definitely doesn't need it, so to be safe we could also make
CLIENT_ALL to CODE|VGA.
And if we agree virtio-mem will need to punch holes on all the bitmaps,
then this patch is even not needed. For that, more below.
I'm guessing whether above could fix the virtio-mem regression after
Hyman's current patch applied.
Said that, IMHO virtio-mem should still use the same helper just like
virtio-balloon as I discussed previously, so as to reset bitmap for the
whole stack (which seems to always be the right thing to do to not miss one
layer of them)?
Well, the difference is that virtio-mem really only gets called exactly once
initially. If we can just reset all bitmaps initially, then virtio-mem
really only has to zap the rb->bmap.
I think virtio-mem should better always punch through, regardless of
whether we can have above change. Consider cases like KVM "initial-all-set"
feature I mentioned above, when that feature bit is set by QEMU, KVM sets
all ones to the bitmap initially. So that may be required for virtio-mem
to clear 1s in KVM at least.