Re: [RFC PATCH v3 19/30] migration/ram: Ignore multifd flush when doing fixed-ram migration
On Wed, Jan 17, 2024 at 03:13:20PM -0300, Fabiano Rosas wrote:
> >> @@ -3242,8 +3243,11 @@ static int ram_save_iterate(QEMUFile *f, void
> >> *opaque)
> >> out:
> >> if (ret >= 0
> >> && migration_is_setup_or_active(migrate_get_current()->state)) {
> >> -if (migrate_multifd() &&
> >> migrate_multifd_flush_after_each_section()) {
> >> -ret =
> >> multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
> >> +if (migrate_multifd() &&
> >> +(migrate_multifd_flush_after_each_section() ||
> >> + migrate_fixed_ram())) {
> >> +ret = multifd_send_sync_main(
> >> +rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
> >
> > Why you want this one? ram_save_iterate() can be called tens for each
> > second iiuc.
> >
>
> AIUI, this is a requirement for live migration, so that we're not
> sending the new version of the page while the old version is still in
> transit.
>
> > There's one more? ram_save_complete():
> >
> > if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
> > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> > }
> >
> > IIUC that's the one you referred to at [1] above, not sure why you modified
> > the code in ram_save_iterate() instead.
> >
>
> I mentioned it in the commit message as well:
>
> " - between iterations, to avoid a slow channel being overrun by a fast
> channel in the subsequent iteration;"
IMHO you only need to flush all threads in find_dirty_block(). That's when
the "real iteration" happens (IOW, when the "real iteration" is defined to
be a full walk across all guest memories, rather than one single call to
ram_save_iterate()).
Multifd used to do too many flushes, and that's why we had the new
migrate_multifd_flush_after_each_section() and it's a bit of a mess if you
see..
To be super safe, you can also flush at ram_save_complete(), but I doubt
its necessity, and this same question applies to generic multifd: the
multifd_send_sync_main() in the ram_save_complete() can be redundant,
afaiu. However we can leave that for later to not add even more dependency
to fixed-ram. If that is justified, we can remove the sync_main for both
socket / file then.
--
Peter Xu
Re: [RFC PATCH v3 19/30] migration/ram: Ignore multifd flush when doing fixed-ram migration
Peter Xu writes:
> On Mon, Nov 27, 2023 at 05:26:01PM -0300, Fabiano Rosas wrote:
>> Some functionalities of multifd are incompatible with the 'fixed-ram'
>> migration format.
>>
>> The MULTIFD_FLUSH flag in particular is not used because in fixed-ram
>> there is no sinchronicity between migration source and destination so
>> there is not need for a sync packet. In fact, fixed-ram disables
>> packets in multifd as a whole.
>>
>> However, we still need to sync the migration thread with the multifd
>> channels at key moments:
>>
>> - between iterations, to avoid a slow channel being overrun by a fast
>> channel in the subsequent iteration;
>>
>> - at ram_save_complete, to make sure all data has been transferred
>> before finishing migration;
>
> [1]
>
>>
>> Make sure RAM_SAVE_FLAG_MULTIFD_FLUSH is only emitted for fixed-ram at
>> those key moments.
>>
>> Signed-off-by: Fabiano Rosas
>> ---
>> migration/ram.c | 12
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 08604222f2..ad6abd1761 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1363,7 +1363,7 @@ static int find_dirty_block(RAMState *rs,
>> PageSearchStatus *pss)
>> pss->page = 0;
>> pss->block = QLIST_NEXT_RCU(pss->block, next);
>> if (!pss->block) {
>> -if (migrate_multifd() &&
>> +if (migrate_multifd() && !migrate_fixed_ram() &&
>> !migrate_multifd_flush_after_each_section()) {
>> QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
>> int ret = multifd_send_sync_main(f);
>> @@ -3112,7 +3112,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>> return ret;
>> }
>>
>> -if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
>> +if (migrate_multifd() && !migrate_multifd_flush_after_each_section()
>> +&& !migrate_fixed_ram()) {
>> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>> }
>>
>> @@ -3242,8 +3243,11 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>> out:
>> if (ret >= 0
>> && migration_is_setup_or_active(migrate_get_current()->state)) {
>> -if (migrate_multifd() &&
>> migrate_multifd_flush_after_each_section()) {
>> -ret =
>> multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
>> +if (migrate_multifd() &&
>> +(migrate_multifd_flush_after_each_section() ||
>> + migrate_fixed_ram())) {
>> +ret = multifd_send_sync_main(
>> +rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
>
> Why you want this one? ram_save_iterate() can be called tens for each
> second iiuc.
>
AIUI, this is a requirement for live migration, so that we're not
sending the new version of the page while the old version is still in
transit.
> There's one more? ram_save_complete():
>
> if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> }
>
> IIUC that's the one you referred to at [1] above, not sure why you modified
> the code in ram_save_iterate() instead.
>
I mentioned it in the commit message as well:
" - between iterations, to avoid a slow channel being overrun by a fast
channel in the subsequent iteration;"
>> if (ret < 0) {
>> return ret;
>> }
>> --
>> 2.35.3
>>
>
> Since the file migration added its whole new code in
> multifd_send_sync_main(), now I'm hesitating whether we should just provide
> multifd_file_sync_threads(), put file sync there, and call explicitly,
> like:
>
> if (migrate_multifd()) {
> if (migrate_is_file()) {
>multifd_file_sync_threads();
> } else if (migrate_multifd_flush_after_each_section()) {
>multifd_send_sync_main();
> }
> }
>
> It'll be much clearer that file goes into its own path and we don't need to
> worry on fat eyes of those if clauses. diff should be similar.
Hm, it could be a good idea indeed. Let me experiment with it.
Re: [RFC PATCH v3 19/30] migration/ram: Ignore multifd flush when doing fixed-ram migration
On Mon, Nov 27, 2023 at 05:26:01PM -0300, Fabiano Rosas wrote:
> Some functionalities of multifd are incompatible with the 'fixed-ram'
> migration format.
>
> The MULTIFD_FLUSH flag in particular is not used because in fixed-ram
> there is no sinchronicity between migration source and destination so
> there is not need for a sync packet. In fact, fixed-ram disables
> packets in multifd as a whole.
>
> However, we still need to sync the migration thread with the multifd
> channels at key moments:
>
> - between iterations, to avoid a slow channel being overrun by a fast
> channel in the subsequent iteration;
>
> - at ram_save_complete, to make sure all data has been transferred
> before finishing migration;
[1]
>
> Make sure RAM_SAVE_FLAG_MULTIFD_FLUSH is only emitted for fixed-ram at
> those key moments.
>
> Signed-off-by: Fabiano Rosas
> ---
> migration/ram.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 08604222f2..ad6abd1761 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1363,7 +1363,7 @@ static int find_dirty_block(RAMState *rs,
> PageSearchStatus *pss)
> pss->page = 0;
> pss->block = QLIST_NEXT_RCU(pss->block, next);
> if (!pss->block) {
> -if (migrate_multifd() &&
> +if (migrate_multifd() && !migrate_fixed_ram() &&
> !migrate_multifd_flush_after_each_section()) {
> QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
> int ret = multifd_send_sync_main(f);
> @@ -3112,7 +3112,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> return ret;
> }
>
> -if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
> +if (migrate_multifd() && !migrate_multifd_flush_after_each_section()
> +&& !migrate_fixed_ram()) {
> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> }
>
> @@ -3242,8 +3243,11 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> out:
> if (ret >= 0
> && migration_is_setup_or_active(migrate_get_current()->state)) {
> -if (migrate_multifd() && migrate_multifd_flush_after_each_section())
> {
> -ret =
> multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
> +if (migrate_multifd() &&
> +(migrate_multifd_flush_after_each_section() ||
> + migrate_fixed_ram())) {
> +ret = multifd_send_sync_main(
> +rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
Why you want this one? ram_save_iterate() can be called tens for each
second iiuc.
There's one more? ram_save_complete():
if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
}
IIUC that's the one you referred to at [1] above, not sure why you modified
the code in ram_save_iterate() instead.
> if (ret < 0) {
> return ret;
> }
> --
> 2.35.3
>
Since the file migration added its whole new code in
multifd_send_sync_main(), now I'm hesitating whether we should just provide
multifd_file_sync_threads(), put file sync there, and call explicitly,
like:
if (migrate_multifd()) {
if (migrate_is_file()) {
multifd_file_sync_threads();
} else if (migrate_multifd_flush_after_each_section()) {
multifd_send_sync_main();
}
}
It'll be much clearer that file goes into its own path and we don't need to
worry on fat eyes of those if clauses. diff should be similar.
--
Peter Xu
[RFC PATCH v3 19/30] migration/ram: Ignore multifd flush when doing fixed-ram migration
Some functionalities of multifd are incompatible with the 'fixed-ram'
migration format.
The MULTIFD_FLUSH flag in particular is not used because in fixed-ram
there is no sinchronicity between migration source and destination so
there is not need for a sync packet. In fact, fixed-ram disables
packets in multifd as a whole.
However, we still need to sync the migration thread with the multifd
channels at key moments:
- between iterations, to avoid a slow channel being overrun by a fast
channel in the subsequent iteration;
- at ram_save_complete, to make sure all data has been transferred
before finishing migration;
Make sure RAM_SAVE_FLAG_MULTIFD_FLUSH is only emitted for fixed-ram at
those key moments.
Signed-off-by: Fabiano Rosas
---
migration/ram.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 08604222f2..ad6abd1761 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1363,7 +1363,7 @@ static int find_dirty_block(RAMState *rs,
PageSearchStatus *pss)
pss->page = 0;
pss->block = QLIST_NEXT_RCU(pss->block, next);
if (!pss->block) {
-if (migrate_multifd() &&
+if (migrate_multifd() && !migrate_fixed_ram() &&
!migrate_multifd_flush_after_each_section()) {
QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
int ret = multifd_send_sync_main(f);
@@ -3112,7 +3112,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
return ret;
}
-if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
+if (migrate_multifd() && !migrate_multifd_flush_after_each_section()
+&& !migrate_fixed_ram()) {
qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
}
@@ -3242,8 +3243,11 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
out:
if (ret >= 0
&& migration_is_setup_or_active(migrate_get_current()->state)) {
-if (migrate_multifd() && migrate_multifd_flush_after_each_section()) {
-ret =
multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
+if (migrate_multifd() &&
+(migrate_multifd_flush_after_each_section() ||
+ migrate_fixed_ram())) {
+ret = multifd_send_sync_main(
+rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
if (ret < 0) {
return ret;
}
--
2.35.3
