Re: [PATCH v7 3/5] migration: Add zero-copy parameter for QMP/HMP for Linux

[Leonardo Bras Soares Passos]

On Wed, Jan 19, 2022 at 3:16 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jan 19, 2022 at 03:03:29PM -0300, Leonardo Bras Soares Passos wrote:
> > Hello Daniel,
> >
> > On Thu, Jan 13, 2022 at 10:10 AM Daniel P. Berrangé <berrange@redhat.com> 
> > wrote:
> > >
> > > On Thu, Jan 06, 2022 at 07:13:40PM -0300, Leonardo Bras wrote:
> > > > Add property that allows zero-copy migration of memory pages,
> > > > and also includes a helper function migrate_use_zero_copy() to check
> > > > if it's enabled.
> > > >
> > > > No code is introduced to actually do the migration, but it allow
> > > > future implementations to enable/disable this feature.
> > > >
> > > > On non-Linux builds this parameter is compiled-out.
> > > >
> > > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > > ---
> > > >  qapi/migration.json   | 24 ++++++++++++++++++++++++
> > > >  migration/migration.h |  5 +++++
> > > >  migration/migration.c | 32 ++++++++++++++++++++++++++++++++
> > > >  migration/socket.c    |  5 +++++
> > > >  monitor/hmp-cmds.c    |  6 ++++++
> > > >  5 files changed, 72 insertions(+)
> > >
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > Thanks!
> >
> > >
> > > >
> > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > index bbfd48cf0b..2e62ea6ebd 100644
> > > > --- a/qapi/migration.json
> > > > +++ b/qapi/migration.json
> > > > @@ -730,6 +730,13 @@
> > > >  #                      will consume more CPU.
> > > >  #                      Defaults to 1. (Since 5.0)
> > > >  #
> > > > +# @zero-copy: Controls behavior on sending memory pages on migration.
> > > > +#             When true, enables a zero-copy mechanism for sending 
> > > > memory
> > > > +#             pages, if host supports it.
> > > > +#             Requires that QEMU be permitted to use locked memory for 
> > > > guest
> > > > +#             RAM pages.
> > > > +#             Defaults to false. (Since 7.0)
> > > > +#
> > > >  # @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
> > > > @@ -769,6 +776,7 @@
> > > >             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> > > >             'max-cpu-throttle', 'multifd-compression',
> > > >             'multifd-zlib-level' ,'multifd-zstd-level',
> > > > +           { 'name': 'zero-copy', 'if' : 'CONFIG_LINUX'},
> > > >             'block-bitmap-mapping' ] }
> > > >
> > > >  ##
> > > > @@ -895,6 +903,13 @@
> > > >  #                      will consume more CPU.
> > > >  #                      Defaults to 1. (Since 5.0)
> > > >  #
> > > > +# @zero-copy: Controls behavior on sending memory pages on migration.
> > > > +#             When true, enables a zero-copy mechanism for sending 
> > > > memory
> > > > +#             pages, if host supports it.
> > > > +#             Requires that QEMU be permitted to use locked memory for 
> > > > guest
> > > > +#             RAM pages.
> > > > +#             Defaults to false. (Since 7.0)
> > > > +#
> > > >  # @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
> > > > @@ -949,6 +964,7 @@
> > > >              '*multifd-compression': 'MultiFDCompression',
> > > >              '*multifd-zlib-level': 'uint8',
> > > >              '*multifd-zstd-level': 'uint8',
> > > > +            '*zero-copy': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
> > > >              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > >
> > > The current zerocopy impl is for the send path.
> > >
> > > Do you expect we might get zerocopy in the receive path
> > > later ?
> >
> > It's possible, but I haven't started the implementation yet.
> >
> > >
> > > If so then either call this 'send-zero-copy', or change it
> > > from a bool to an enum taking '["send", "recv", "both"]'.
> > >
> > > I'd probably take the former and just rename it.
> > >
> >
> > Well, my rationale:
> > - I want to set zero copy sending:
> > zero-copy is set in the sending host, start migration.
> >
> > - I want to set zero copy receiving:
> > zero-copy is set in the receiving host, wait for migration.
> > (Of course host support is checked when setting the parameter).
> >
> > The problem with the current approach is trying to enable zero-copy on
> > receive before it's implemented, which will 'fail' silently .
> > A possible solution would be to add a patch to check in the receiving
> > path if zero-copy is enabled, and fail for now.
>
> That's not good because mgmt apps cannot query the QAPI schema
> to find out if this feature is supported or not.
>
> If we wantt o support zero copy recv, then we need an explicit
> flag for it that is distinct from zero copy send, so that apps
> can introspect whether the feature is implemneted in QEMU or
> not.
>
> Distinct named bool flags feels better, and also makes it clear
> to anyone not familiar with the impl that the current  code is
> strictly send only.
>

Ok, I see the point.
I will try to refactor the code changing zero-copy to zero-copy-send
or something like that.

Best regards,
Leo