| Message ID | 20230518171304.95006-10-quintela@redhat.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [PULL,01/12] configure: add --disable-colo-proxy option | expand |
Am 18.05.23 um 19:13 schrieb Juan Quintela: > diff --git a/migration/migration-stats.c b/migration/migration-stats.c > index feec7d7369..97759a45f3 100644 > --- a/migration/migration-stats.c > +++ b/migration/migration-stats.c > @@ -24,7 +24,9 @@ bool migration_rate_exceeded(QEMUFile *f) > return true; > } > > - uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); > + uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start); > + uint64_t rate_limit_current = migration_transferred_bytes(f); > + uint64_t rate_limit_used = rate_limit_current - rate_limit_start; > uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); > > if (rate_limit_max == RATE_LIMIT_DISABLED) { Hi, just wanted to let you know that the call to migration_transferred_bytes(f) here can introduce a huge performance penalty when taking a snapshot. I ran into the issue when testing something else, with a single-disk snapshot. Without this call it takes about two seconds, with the call about two minutes. Best Regards, Fiona
Fiona Ebner <f.ebner@proxmox.com> wrote: > Am 18.05.23 um 19:13 schrieb Juan Quintela: >> diff --git a/migration/migration-stats.c b/migration/migration-stats.c >> index feec7d7369..97759a45f3 100644 >> --- a/migration/migration-stats.c >> +++ b/migration/migration-stats.c >> @@ -24,7 +24,9 @@ bool migration_rate_exceeded(QEMUFile *f) >> return true; >> } >> >> - uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); >> + uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start); >> + uint64_t rate_limit_current = migration_transferred_bytes(f); >> + uint64_t rate_limit_used = rate_limit_current - rate_limit_start; >> uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); >> >> if (rate_limit_max == RATE_LIMIT_DISABLED) { > > Hi, > just wanted to let you know that the call to > migration_transferred_bytes(f) here can introduce a huge performance > penalty when taking a snapshot. I ran into the issue when testing > something else, with a single-disk snapshot. Without this call it takes > about two seconds, with the call about two minutes. Ouch. Now that everything is reviewed for that series I can sent the new set of patches. As I drop the counter it should just get the speed back. New series comming that removed rate_limit counter altogether. Can you take a look after I send it? Thanks for the report. And now that we are at it. How are you testing this? As you appears to be the bigger user of snapshots (or at least the louder). Creating tests/qtest/snapshot-test.c could be a good idea. 1st to check this kind of breakage. 2nd so I be sure that we don't "pesimize" your use case. Hint, hint. 2 seconds vs 2 minutes. A more detailed explanation that what are you doing would be great. I.e. you are taking lots of snapshots by second or what? Later, Juan.
Am 26.05.23 um 10:55 schrieb Juan Quintela: > Fiona Ebner <f.ebner@proxmox.com> wrote: >> Am 18.05.23 um 19:13 schrieb Juan Quintela: >>> diff --git a/migration/migration-stats.c b/migration/migration-stats.c >>> index feec7d7369..97759a45f3 100644 >>> --- a/migration/migration-stats.c >>> +++ b/migration/migration-stats.c >>> @@ -24,7 +24,9 @@ bool migration_rate_exceeded(QEMUFile *f) >>> return true; >>> } >>> >>> - uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); >>> + uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start); >>> + uint64_t rate_limit_current = migration_transferred_bytes(f); >>> + uint64_t rate_limit_used = rate_limit_current - rate_limit_start; >>> uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); >>> >>> if (rate_limit_max == RATE_LIMIT_DISABLED) { >> >> Hi, >> just wanted to let you know that the call to >> migration_transferred_bytes(f) here can introduce a huge performance >> penalty when taking a snapshot. I ran into the issue when testing >> something else, with a single-disk snapshot. Without this call it takes >> about two seconds, with the call about two minutes. > > Ouch. > > Now that everything is reviewed for that series I can sent the new set > of patches. As I drop the counter it should just get the speed back. > > New series comming that removed rate_limit counter altogether. > > Can you take a look after I send it?> Sure. Please CC me, so I don't forget :) > Thanks for the report. > > And now that we are at it. How are you testing this? I ran into it while testing the "BQL during save setup" patch [0] on (at that time current) master. > As you appears to be the bigger user of snapshots (or at least the > louder). We do have quite a few users using snapshots, but it's actually not the upstream snapshot-save, but a custom QMP command based on it and migration code-wise. We do keep the VM running while taking the snapshot, and write the state to a new raw file/block device. When it's ready, the VM is stopped, the management software takes snapshots of the disks (via QEMU for qcow2/RBD and via the storage layer for other storages) and then the VM is resumed. The caveat is that the saved state will be the one right before the snapshot finished, not the one when the snapshot started (as with background migration). If you really want to see the historically grown details: [1]. It's much older than background-snapshot and we probably could switch to using background-snapshot with some (minor) changes. I do plan to evaluate that at some point, but haven't had the time yet. > Creating tests/qtest/snapshot-test.c could be a good idea. I can try to take a look at adding tests at some point, but I will be very busy in the near future, because we have releases coming up. So if anybody else wants to do it, don't wait for me :) > 1st to check this kind of breakage. > 2nd so I be sure that we don't "pesimize" your use case. > > Hint, hint. > > 2 seconds vs 2 minutes. > > A more detailed explanation that what are you doing would be great. > I.e. you are taking lots of snapshots by second or what? No, it was just a single snapshot on a single drive for a VM with 2GiB of RAM, nothing special. I also just "measured" it by counting, I thought exact values won't matter when it's such a big difference. Here's the command line: > ./qemu-system-x86_64 \ > -chardev 'socket,id=qmp,path=/var/run/qemu-server/106.qmp,server=on,wait=off' \ > -mon 'chardev=qmp,mode=control' \ > -pidfile /var/run/qemu-server/106.pid \ > -smp '2,sockets=2,cores=1,maxcpus=2' \ > -nodefaults \ > -vnc 'unix:/var/run/qemu-server/106.vnc,password=on' \ > -m 2048 \ > -device 'VGA,id=vga,bus=pci.0,addr=0x2' \ > -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \ > -drive 'file=/var/lib/vz/images/106/vm-106-disk-1.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on,node-name=scsi0' \ > -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \ > -machine 'type=pc' and QMP params: > "snapshot-save", > 'job-id' => "save$i", > tag => 'snap0', > vmstate => 'scsi0', > devices => ['scsi0'], [0]: https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg06541.html [1]: https://git.proxmox.com/?p=pve-qemu.git;a=blob;f=debian/patches/pve/0018-PVE-add-savevm-async-for-background-state-snapshots.patch;h=d42d0f02f270934610b1ac90b2813b5ee617427d;hb=bd3c1fa52561e4a307e5b5b37754788408fc75a6 Best Regards, Fiona
Am 23.05.23 um 14:31 schrieb Fiona Ebner: > Am 18.05.23 um 19:13 schrieb Juan Quintela: >> diff --git a/migration/migration-stats.c b/migration/migration-stats.c >> index feec7d7369..97759a45f3 100644 >> --- a/migration/migration-stats.c >> +++ b/migration/migration-stats.c >> @@ -24,7 +24,9 @@ bool migration_rate_exceeded(QEMUFile *f) >> return true; >> } >> >> - uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); >> + uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start); >> + uint64_t rate_limit_current = migration_transferred_bytes(f); >> + uint64_t rate_limit_used = rate_limit_current - rate_limit_start; >> uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); >> >> if (rate_limit_max == RATE_LIMIT_DISABLED) { > > Hi, > just wanted to let you know that the call to > migration_transferred_bytes(f) here can introduce a huge performance > penalty when taking a snapshot. I ran into the issue when testing > something else, with a single-disk snapshot. Without this call it takes > about two seconds, with the call about two minutes. > > Unfortunately, the regression is still in current master and v8.1-rc1. Did the v2 [0] of the series never make it? [0]: https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg07708.html Best Regards, Fiona
diff --git a/migration/migration-stats.h b/migration/migration-stats.h index 827ea80c9b..4c4daa2e97 100644 --- a/migration/migration-stats.h +++ b/migration/migration-stats.h @@ -81,6 +81,10 @@ typedef struct { * Number of bytes sent during precopy stage. */ Stat64 precopy_bytes; + /* + * Amount of transferred data at the start of current cycle. + */ + Stat64 rate_limit_start; /* * Maximum amount of data we can send in a cycle. */ @@ -122,8 +126,10 @@ uint64_t migration_rate_get(void); * migration_rate_reset: Reset the rate limit counter. * * This is called when we know we start a new transfer cycle. + * + * @f: QEMUFile used for main migration channel */ -void migration_rate_reset(void); +void migration_rate_reset(QEMUFile *f); /** * migration_rate_set: Set the maximum amount that can be transferred. diff --git a/migration/migration-stats.c b/migration/migration-stats.c index feec7d7369..97759a45f3 100644 --- a/migration/migration-stats.c +++ b/migration/migration-stats.c @@ -24,7 +24,9 @@ bool migration_rate_exceeded(QEMUFile *f) return true; } - uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); + uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start); + uint64_t rate_limit_current = migration_transferred_bytes(f); + uint64_t rate_limit_used = rate_limit_current - rate_limit_start; uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); if (rate_limit_max == RATE_LIMIT_DISABLED) { @@ -51,9 +53,10 @@ void migration_rate_set(uint64_t limit) stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO); } -void migration_rate_reset(void) +void migration_rate_reset(QEMUFile *f) { stat64_set(&mig_stats.rate_limit_used, 0); + stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f)); } void migration_rate_account(uint64_t len) diff --git a/migration/migration.c b/migration/migration.c index 952100c8d7..5de7f734b9 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2692,7 +2692,7 @@ static void migration_update_counters(MigrationState *s, stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth; } - migration_rate_reset(); + migration_rate_reset(s->to_dst_file); update_iteration_initial_status(s);