Re: [PATCH 10/20] qemu: Add support for mapped-ram on save

Daniel P. Berrangé <berrange(a)redhat.com&gt; writes: Hmm, I'm thinking this might be caused by zero page handling. The non mapped-ram path has an extra buffer_is_zero() and memset() of the hva page. Now, is it an issue that mapped-ram skips that memset? I assume guest memory will always be clear at the start of migration. There won't be a situation where the destination VM starts with memory already dirty... *and* the save file is also different, otherwise it wouldn't make any difference. > >> -----------------------+---------+---------+--------------+-------- >> mapped-ram + direct IO | | | | >> + multifd-channels=8 | 4.421s | 0.845s | 34368554318 | 1774312 >> ------------------------------------------------------------------- >> >> VM: 32G RAM, 30G dirty, 1 vcpu in tight loop dirtying memory >> >> | save | restore | >> | time | time | Size | Blocks >> -----------------------+---------+---------+--------------+--------- >> legacy | 25.800s | 14.332s | 33154309983 | 64754512 >> -----------------------+---------+---------+--------------+--------- >> mapped-ram | 18.742s | 15.027s | 34368559228 | 64617160 >> -----------------------+---------+---------+--------------+--------- >> legacy + direct IO | 13.115s | 18.050s | 33154310496 | 64754520 >> -----------------------+---------+---------+--------------+--------- >> mapped-ram + direct IO | 13.623s | 15.959s | 34368557392 | 64662040 > > These figures make more sense with restore time matching save time > more or less. > >> -----------------------+-------- +---------+--------------+--------- >> mapped-ram + direct IO | | | | >> + multifd-channels=8 | 6.994s | 6.470s | 34368554980 | 64665776 >> -------------------------------------------------------------------- >> >> As can be seen from the tables, one caveat of mapped-ram is the logical >> file size of a saved image is basically equivalent to the VM memory size. >> Note however that mapped-ram typically uses fewer blocks on disk. >> >> Another caveat of mapped-ram is the requirement for a seekable file >> descriptor, which currently makes it incompatible with libvirt's >> support for save image compression. Also note the mapped-ram stream >> is incompatible with the existing stream format, hence mapped-ram >> cannot be used to restore an image saved with the existing format >> and vice versa. >> >> [1] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m... >> >> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >> --- >> src/qemu/qemu_driver.c | 20 ++++-- >> src/qemu/qemu_migration.c | 139 ++++++++++++++++++++++++++------------ >> src/qemu/qemu_migration.h | 4 +- >> src/qemu/qemu_monitor.c | 36 ++++++++++ >> src/qemu/qemu_monitor.h | 4 ++ >> src/qemu/qemu_saveimage.c | 43 +++++++++--- >> src/qemu/qemu_saveimage.h | 2 + >> src/qemu/qemu_snapshot.c | 9 ++- >> 8 files changed, 195 insertions(+), 62 deletions(-) >> > > > >> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c >> index 6f2ce40124..98a1ad638d 100644 >> --- a/src/qemu/qemu_saveimage.c >> +++ b/src/qemu/qemu_saveimage.c >> @@ -96,6 +96,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree); >> */ >> virQEMUSaveData * >> virQEMUSaveDataNew(virQEMUDriver *driver, >> + virDomainObj *vm, >> char *domXML, >> qemuDomainSaveCookie *cookieObj, >> bool running, >> @@ -115,6 +116,19 @@ virQEMUSaveDataNew(virQEMUDriver *driver, >> header = &data->header; >> memcpy(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)); >> header->version = cfg->saveImageVersion; >> + >> + /* Enable mapped-ram feature if available and save version >= 3 */ >> + if (header->version >= QEMU_SAVE_VERSION && >> + qemuMigrationCapsGet(vm, QEMU_MIGRATION_CAP_MAPPED_RAM)) { >> + if (compressed != QEMU_SAVE_FORMAT_RAW) { >> + virReportError(VIR_ERR_OPERATION_FAILED, >> + _("compression is not supported with save image version %1$u"), >> + header->version); >> + goto error; >> + } >> + header->features |= QEMU_SAVE_FEATURE_MAPPED_RAM; >> + } > > If the QEMU we're usnig doesnt have CAP_MAPPED_RAM, then I think > we should NOT default to Version 3 save images, as that's creating > a backcompat problem for zero user benefit. > > This suggests that in qemu_conf.c, we should initialize the > default value to '0', and then in this code, if we see > version 0 we should pick either 2 or 3 depending on mapped > ram. > >> + >> header->was_running = running ? 1 : 0; >> header->compressed = compressed; >> > > With regards, > Daniel