Re: [PATCH v4 3/3] multifd: Implement zerocopy write in multifd migration (multifd-zerocopy)

[Leonardo Bras Soares Passos]

Hello Markus!
(comments at the bottom)

On Tue, Oct 12, 2021 at 2:54 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Leonardo Bras Soares Passos <leobras@redhat.com> writes:
>
> > Hello Eric,
> >
> > On Mon, Oct 11, 2021 at 4:32 PM Eric Blake <eblake@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 <leobras@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