Hello Markus!
(comments at the bottom)
On Tue, Oct 12, 2021 at 2:54 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Leonardo Bras Soares Passos <leob...@redhat.com> writes:
>
> > Hello Eric,
> >
> > On Mon, Oct 11, 2021 at 4:32 PM Eric Blake <ebl...@redhat.com> wrote:
> >>
> >> On Sat, Oct 09, 2021 at 04:56:13AM -0300, Leonardo Bras wrote:
> >> > Implement zerocopy on nocomp_send_write(), by making use of QIOChannel
> >> > zerocopy interface.
> >> >
> >> > Change multifd_send_sync_main() so it can distinguish the last sync from
> >> > the setup and per-iteration ones, so a flush_zerocopy() can be called
> >> > at the last sync in order to make sure all RAM is sent before finishing
> >> > the migration.
> >> >
> >> > Also make it return -1 if flush_zerocopy() fails, in order to cancel
> >> > the migration process, and avoid resuming the guest in the target host
> >> > without receiving all current RAM.
> >> >
> >> > This will work fine on RAM migration because the RAM pages are not
> >> > usually freed,
> >> > and there is no problem on changing the pages content between
> >> > async_send() and
> >> > the actual sending of the buffer, because this change will dirty the
> >> > page and
> >> > cause it to be re-sent on a next iteration anyway.
> >> >
> >> > Given a lot of locked memory may be needed in order to use multid
> >> > migration
> >> > with zerocopy enabled, make it optional by creating a new parameter
> >> > multifd-zerocopy on qapi, so low-privileged users can still perform
> >> > multifd
> >> > migrations.
> >> >
> >> > Signed-off-by: Leonardo Bras <leob...@redhat.com>
> >> > ---
> >> > qapi/migration.json | 18 ++++++++++++++++++
> >> > migration/migration.h | 1 +
> >> > migration/multifd.h | 2 +-
> >> > migration/migration.c | 20 ++++++++++++++++++++
> >> > migration/multifd.c | 33 ++++++++++++++++++++++++++++-----
> >> > migration/ram.c | 20 +++++++++++++-------
> >> > monitor/hmp-cmds.c | 4 ++++
> >> > 7 files changed, 85 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/qapi/migration.json b/qapi/migration.json
> >> > index 88f07baedd..c4890cbb54 100644
> >> > --- a/qapi/migration.json
> >> > +++ b/qapi/migration.json
> >> > @@ -724,6 +724,11 @@
> >> > # will consume more CPU.
> >> > # Defaults to 1. (Since 5.0)
> >> > #
> >> > +# @multifd-zerocopy: Controls behavior on sending memory pages on
> >> > multifd migration.
> >> > +# When true, enables a zerocopy mechanism for
> >> > sending memory
> >> > +# pages, if host does support it.
> >>
> >> s/does support/supports/ (several instances this patch)
> >
> > I will make sure to fix that in v5.
> >
> >>
> >> > +# Defaults to false. (Since 6.2)
> >> > +#
> >> > # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> >> > # aliases for the purpose of dirty bitmap
> >> > migration. Such
> >> > # aliases may for example be the corresponding
> >> > names on the
> >> > @@ -758,6 +763,7 @@
> >> > 'xbzrle-cache-size', 'max-postcopy-bandwidth',
> >> > 'max-cpu-throttle', 'multifd-compression',
> >> > 'multifd-zlib-level' ,'multifd-zstd-level',
> >> > + 'multifd-zerocopy',
> >> > 'block-bitmap-mapping' ] }
> >>
> >> Should this feature be guarded with 'if':'CONFIG_LINUX', since that's
> >> the only platform where you even compile the code (even then, it can
> >> still fail if the kernel doesn't support it).
> >
> > I think it makes sense for the feature to always be available, even
> > though it's not supported
> > outside linux > v4.14.
> >
> > IMHO it makes more sense for the user to get an error when it starts
> > migration, due to host
> > not supporting zerocopy, than the error happening in the config step,
> > which may cause the user
> > to question if it was the right parameter.
> >
> > The config message error here could also be ignored, and users can
> > think zerocopy is working, while it's not.
> >
> > For automated migrations, this feature should never be enabled for
> > non-linux / older linux hosts anyway.
> >
> > Is there a good argument I am missing for this feature being disabled
> > on non-linux?
>
> The general argument for having QAPI schema 'if' mirror the C
> implementation's #if is introspection. Let me explain why that matters.
>
> Consider a management application that supports a range of QEMU
> versions, say 5.0 to 6.2. Say it wants to use an QMP command that is
> new in QEMU 6.2. The sane way to do that is to probe for the command
> with query-qmp-schema. Same for command arguments, and anything else
> QMP.
>
> If you doubt "sane", check out Part II of "QEMU interface introspection:
> From hacks to solutions"[*].
>
> The same technique works when a QMP command / argument / whatever is
> compile-time conditional ('if' in the schema). The code the management
> application needs anyway to deal with older QEMU now also deals with
> "compiled out". Nice.
>
> Of course, a command or argument present in QEMU can still fail, and the
> management application still needs to handle failure. Distinguishing
> different failure modes can be bothersome and/or fragile.
>
> By making the QAPI schema conditional mirror the C conditional, you
> squash the failure mode "this version of QEMU supports it, but this
> build of QEMU does not" into "this version of QEMU does not support
> it". Makes sense, doesn't it?
>
> A minor additional advantage is less generated code.
>
>
>
> [*]
> http://events17.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf
>
This was very informative, thanks!
I now understand the rationale about this choice.
TBH I am not very used to this syntax.
I did a take a peek at some other json files, and ended adding this
lines in code, which compiled just fine:
for : enum MigrationParameter
{'name': 'multifd-zerocopy', 'if' : 'CONFIG_LINUX'},
for : struct MigrateSetParameters and struct MigrationParameters:
'*multifd-zerocopy': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
Is that enough? Is there any other necessary change?
Thanks for reviewing and for helping out with this!
Best regards,
Leonardo Bras
