Re: [PATCH v17 08/10] net/tap: support local migration with virtio-net

[Markus Armbruster <armbru@redhat.com>]

[Cc: John Snow for advice on cross-references (search for "John?")]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Support transferring of TAP state (including open fd).
>
> Add new property "local-migration-supported", which defines,
> is local-migration is actually supported for this TAP device.

which defines whether

> Starting from 11.1 MT it's enabled by default.

What does "MT" mean?

>
> Note, that local-migration (including migrating opened FDs
> through migration channel, which must be UNIX socket), is
> enabled by global "local" migration parameters. But individual
> devices may have additional options to enable/disable it
> personally.

per device

>
> Add new option "incoming-fds", which should be set to true on
> target for incoming migration work. It says "do not open any
> files, but instead wait for FDs coming from migration stream".
> "local-migration-supported" option is not enough, as it work in pair
> with migration parameter "local", and intialization process
> of TAP device should not depend on migration parameters.
>
> For new option require explicitly unset script and downscript,
> to keep possibility of implementing support for them in the
> future.

Why explicitly unset?  Do we have to override a default script?

> Note disabling read polling on source stop for TAP migration:

I don't understand this sentence.

> otherwise, source process may steal packages from TAP fd even
> after source vm STOP.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

> diff --git a/qapi/net.json b/qapi/net.json
> index 1a6382825c5..c5d87ba308b 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -425,6 +425,22 @@
   ##
   # @NetdevTapOptions:
   #
   # Used to configure a host TAP network interface backend.
   #
   # @ifname: interface name
   #
   # @fd: file descriptor of an already opened tap
   #
   # @fds: multiple file descriptors of already opened multiqueue capable
   #     tap
   #
   # @script: script to initialize the interface
   #
   # @downscript: script to shut down the interface
   #
   # @br: bridge name (since 2.8)
   #
   # @helper: command to execute to configure bridge
   #
   # @sndbuf: send buffer limit.  Understands [TGMKkb] suffixes.
   #
   # @vnet_hdr: enable the IFF_VNET_HDR flag on the tap interface
   #
   # @vhost: enable vhost-net network accelerator
   #
   # @vhostfd: file descriptor of an already opened vhost net device
   #
   # @vhostfds: file descriptors of multiple already opened vhost net
   #     devices
   #
   # @vhostforce: vhost on for non-MSIX virtio guests
   #
   # @queues: number of queues to be created for multiqueue capable tap
   #
>  # @poll-us: maximum number of microseconds that could be spent on busy
>  #     polling for tap (since 2.7)
>  #
> +# @incoming-fds: do not open or create any TAP devices.  Prepare for
> +#     getting TAP file descriptors from incoming migration stream.
> +#     The option is incompatible with any of @fd, @fds, @helper, @br,
> +#     @ifname, @sndbuf and @vnet_hdr options, and requires @script and
> +#     @downscript be explicitly set to nothing (empty string or "no"),

WAT?!?

Special value "no" is not documented in the QAPI schema, see the
descriptions of @script and @downscript above.

It is documented for -netdev tap in qemu-options.hx:

    ``-netdev tap,id=id[,fd=h][,ifname=name][,script=file][,downscript=dfile][,br=bridge][,helper=helper]``
        Configure a host TAP network backend with ID id.

        Use the network script file to configure it and the network script
        dfile to deconfigure it. If name is not provided, the OS
        automatically provides one. The default network configure script is
        ``/etc/qemu-ifup`` and the default network deconfigure script is
        ``/etc/qemu-ifdown``. Use ``script=no`` or ``downscript=no`` to
        disable script execution.

Special value "" is okay: you can't name have a script named "".

Special value "no" isn't: you can have a script named "no", but if you
try to use it, it's silently ignored.  Yes, it's a silly name, but it's
also a silly interface.

I think we should document "no" properly, and also deprecate it.

> +#     and requires also @local-migration-supported to be true, "local"
> +#     migration parameter be set as well, and QEMU being in incoming
> +#     migration state.  (Since 11.1)

This sentence is rather long and hard to parse.  Here's my attempt:

   #     The option is incompatible with any of @fd, @fds, @helper, @br,
   #     @ifname, @sndbuf and @vnet_hdr options.  @script and @downscript
   #     must be explicitly disabled (empty string or "no"), and
   #     @local-migration-supported must be true.  Additionally,
   #     migration parameter @local must be set, and QEMU mist be in
   #     incoming migration state.  (Since 11.1)

Ideally, "migration parameter @local" would link to its description, but
I can't tell you offhand whether we can do that and how.  John?

> +#
> +# @local-migration-supported: enable local migration for this TAP
> +#     backend.  When set, local migration is enabled/disabled by
> +#     "local" migration parameter for this TAP backend.  When unset,

migration parameter @local

> +#     "local" migration parameter is ignored for this TAP backend.
> +#     (Since 11.1.  Defaults to true for MT >= 11.1, and to false for
> +#     MT < 11.1)

What's "MT"?

> +#
>  # Since: 1.2
>  ##
>  { 'struct': 'NetdevTapOptions',
> @@ -443,7 +459,9 @@
>      '*vhostfds':   'str',
>      '*vhostforce': 'bool',
>      '*queues':     'uint32',
> -    '*poll-us':    'uint32'} }
> +    '*poll-us':    'uint32',
> +    '*incoming-fds': 'bool',
> +    '*local-migration-supported': 'bool' } }
>  
>  ##
>  # @NetdevSocketOptions:

Two bools mean four cases.  @incoming-fd's description excludes the case
"incoming-fd": true, "local-migration-supported": false.  Awkward.  Does
case ""incoming-fd": false, "local-migration-supported": true make sense
and why?