Re: [PATCH v13 6/8] net/tap: support local migration with virtio-net

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

On 24.03.26 15:33, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Support transferring of TAP state (including open fd) through
>> migration stream as part of viritio-net "local-migration".
>>
>> Add new option, incoming-fds, which should be set to true to
>> trigger new logic.
>>
>> For new option require explicitly unset script and downscript,
>> to keep possibility of implementing support for them in future.
>>
>> Note disabling read polling on source stop for TAP migration:
>> otherwise, source process may steal packages from TAP fd even
>> after source vm STOP.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   net/tap.c     | 147 +++++++++++++++++++++++++++++++++++++++++++++++---
>>   qapi/net.json |   7 ++-
>>   2 files changed, 147 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 9d6213fc3e5..2156b6cbb73 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -36,6 +36,7 @@
>>   #include "net/net.h"
>>   #include "clients.h"
>>   #include "monitor/monitor.h"
>> +#include "system/runstate.h"
>>   #include "system/system.h"
>>   #include "qapi/error.h"
>>   #include "qemu/cutils.h"
>> @@ -86,6 +87,9 @@ typedef struct TAPState {
>>       VHostNetState *vhost_net;
>>       unsigned host_vnet_hdr_len;
>>       Notifier exit;
>> +
>> +    bool read_poll_detached;
>> +    VMChangeStateEntry *vmstate;
>>   } TAPState;
>>   
>>   static void launch_script(const char *setup_script, const char *ifname,
>> @@ -94,19 +98,25 @@ static void launch_script(const char *setup_script, const char *ifname,
>>   static void tap_send(void *opaque);
>>   static void tap_writable(void *opaque);
>>   
>> +static bool tap_is_explicit_no_scirpt(const char *script_arg)
> 
> "scirpt"?  Do you mean "script"?
> 
>> +{
>> +    return script_arg &&
>> +        (script_arg[0] == '\0' || strcmp(script_arg, "no") == 0);
>> +}
>> +
>>   static char *tap_parse_script(const char *script_arg, const char *default_path)
>>   {
>>       g_autofree char *res = g_strdup(script_arg);
>>   
>> -    if (!res) {
>> -        res = get_relocated_path(default_path);
>> +    if (tap_is_explicit_no_scirpt(script_arg)) {
>> +        return NULL;
>>       }
>>   
>> -    if (res[0] == '\0' || strcmp(res, "no") == 0) {
>> -        return NULL;
>> +    if (!script_arg) {
>> +        return get_relocated_path(default_path);
>>       }
>>   
>> -    return g_steal_pointer(&res);
>> +    return g_strdup(script_arg);
>>   }
>>   
>>   static void tap_update_fd_handler(TAPState *s)
>> @@ -123,6 +133,23 @@ static void tap_read_poll(TAPState *s, bool enable)
>>       tap_update_fd_handler(s);
>>   }
>>   
>> +static void tap_vm_state_change(void *opaque, bool running, RunState state)
>> +{
>> +    TAPState *s = opaque;
>> +
>> +    if (running) {
>> +        if (s->read_poll_detached) {
>> +            tap_read_poll(s, true);
>> +            s->read_poll_detached = false;
>> +        }
>> +    } else if (state == RUN_STATE_FINISH_MIGRATE) {
>> +        if (s->read_poll) {
>> +            s->read_poll_detached = true;
>> +            tap_read_poll(s, false);
>> +        }
>> +    }
>> +}
>> +
>>   static void tap_write_poll(TAPState *s, bool enable)
>>   {
>>       s->write_poll = enable;
>> @@ -353,6 +380,11 @@ static void tap_cleanup(NetClientState *nc)
>>           s->exit.notify = NULL;
>>       }
>>   
>> +    if (s->vmstate) {
>> +        qemu_del_vm_change_state_handler(s->vmstate);
>> +        s->vmstate = NULL;
>> +    }
>> +
>>       tap_read_poll(s, false);
>>       tap_write_poll(s, false);
>>       close(s->fd);
>> @@ -393,6 +425,65 @@ static VHostNetState *tap_get_vhost_net(NetClientState *nc)
>>       return s->vhost_net;
>>   }
>>   
>> +static bool tap_is_wait_incoming(NetClientState *nc)
>> +{
>> +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
>> +    assert(nc->info->type == NET_CLIENT_DRIVER_TAP);
>> +    return s->fd == -1;
>> +}
>> +
>> +static int tap_pre_load(void *opaque)
>> +{
>> +    TAPState *s = opaque;
>> +
>> +    if (s->fd != -1) {
>> +        error_report(
>> +            "TAP is already initialized and cannot receive incoming fd");
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static bool tap_setup_vhost(TAPState *s, Error **errp);
>> +
>> +static int tap_post_load(void *opaque, int version_id)
>> +{
>> +    TAPState *s = opaque;
>> +    Error *local_err = NULL;
>> +
>> +    tap_read_poll(s, true);
>> +
>> +    if (s->fd < 0) {
>> +        return -1;
>> +    }
>> +
>> +    if (!tap_setup_vhost(s, &local_err)) {
>> +        error_prepend(&local_err,
>> +                      "Failed to setup vhost during TAP post-load: ");
>> +        error_report_err(local_err);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_tap = {
>> +    .name = "net-tap",
>> +    .pre_load = tap_pre_load,
>> +    .post_load = tap_post_load,
>> +    .fields = (const VMStateField[]) {
>> +        VMSTATE_FD(fd, TAPState),
>> +        VMSTATE_BOOL(using_vnet_hdr, TAPState),
>> +        VMSTATE_BOOL(has_ufo, TAPState),
>> +        VMSTATE_BOOL(has_uso, TAPState),
>> +        VMSTATE_BOOL(has_tunnel, TAPState),
>> +        VMSTATE_BOOL(enabled, TAPState),
>> +        VMSTATE_UINT32(host_vnet_hdr_len, TAPState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   /* fd support */
>>   
>>   static NetClientInfo net_tap_info = {
>> @@ -412,7 +503,9 @@ static NetClientInfo net_tap_info = {
>>       .set_vnet_le = tap_set_vnet_le,
>>       .set_vnet_be = tap_set_vnet_be,
>>       .set_steering_ebpf = tap_set_steering_ebpf,
>> +    .is_wait_incoming = tap_is_wait_incoming,
>>       .get_vhost_net = tap_get_vhost_net,
>> +    .backend_vmsd = &vmstate_tap,
>>   };
>>   
>>   static TAPState *net_tap_fd_init(NetClientState *peer,
>> @@ -748,6 +841,9 @@ static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>       int sndbuf =
>>           (tap->has_sndbuf && tap->sndbuf) ? MIN(tap->sndbuf, INT_MAX) : INT_MAX;
>>   
>> +    s->read_poll_detached = false;
>> +    s->vmstate = qemu_add_vm_change_state_handler(tap_vm_state_change, s);
>> +
>>       if (!tap_set_sndbuf(fd, sndbuf, sndbuf_required ? errp : NULL) &&
>>           sndbuf_required) {
>>           goto failed;
>> @@ -779,6 +875,8 @@ static bool net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>       return true;
>>   
>>   failed:
>> +    qemu_del_vm_change_state_handler(s->vmstate);
>> +    s->vmstate = NULL;
>>       qemu_del_net_client(&s->nc);
>>       return false;
>>   }
>> @@ -910,6 +1008,26 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>           return -1;
>>       }
>>   
>> +    if (tap->incoming_fds &&
>> +        (tap->fd || tap->fds || tap->helper || tap->br || tap->ifname ||
>> +         tap->has_sndbuf || tap->has_vnet_hdr)) {
>> +        error_setg(errp, "incoming-fds is incompatible with "
>> +                   "fd=, fds=, helper=, br=, ifname=, sndbuf= and vnet_hdr=");
> 
> @incoming-fds excludes certain optional members, and ...
> 
>> +        return -1;
>> +    }
>> +
>> +    if (tap->incoming_fds &&
>> +        !(tap_is_explicit_no_scirpt(tap->script) &&
>> +          tap_is_explicit_no_scirpt(tap->downscript))) {
>> +        /*
>> +         * script="" and downscript="" are silently supported to be consistent
>> +         * with cases without incoming_fds, but do not care to put this into
>> +         * error message.
>> +         */
>> +        error_setg(errp, "incoming-fds requires script=no and downscript=no");
> 
> ... requires others.  Not documented in net.json.  Should it be?

Hmm. Excluded options are not documented as well, and as many other relations
between options around TAP.

Still, I script/downscript requirements are more unobvious, so, let's add it to doc.

> 
>> +        return -1;
>> +    }
>> +
>>       queues = tap_parse_fds_and_queues(tap, &fds, errp);
>>       if (queues < 0) {
>>           return -1;
>> @@ -928,7 +1046,24 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>           goto fail;
>>       }
>>   
>> -    if (fds) {
>> +    if (tap->incoming_fds) {
>> +        for (i = 0; i < queues; i++) {
>> +            NetClientState *nc;
>> +            TAPState *s;
>> +
>> +            nc = qemu_new_net_client(&net_tap_info, peer, "tap", name);
>> +            qemu_set_info_str(nc, "incoming");
>> +
>> +            s = DO_UPCAST(TAPState, nc, nc);
>> +            s->fd = -1;
>> +            if (vhost_fds) {
>> +                s->vhostfd = vhost_fds[i];
>> +                s->vhost_busyloop_timeout = tap->has_poll_us ? tap->poll_us : 0;
>> +            } else {
>> +                s->vhostfd = -1;
>> +            }
>> +        }
>> +    } else if (fds) {
>>           for (i = 0; i < queues; i++) {
>>               if (i == 0) {
>>                   vnet_hdr = tap_probe_vnet_hdr(fds[i], errp);
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 118bd349651..2240de7dbf6 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -355,6 +355,10 @@
>>   # @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 opened TAP file descriptors from incoming migration
>> +#     stream.  (Since 11.0)
> 
> Let's scratch "opened".
> 
> Sure you're still targeting 11.0?

Will fix

> 
>> +#
>>   # Since: 1.2
>>   ##
>>   { 'struct': 'NetdevTapOptions',
>> @@ -373,7 +377,8 @@
>>       '*vhostfds':   'str',
>>       '*vhostforce': 'bool',
>>       '*queues':     'uint32',
>> -    '*poll-us':    'uint32'} }
>> +    '*poll-us':    'uint32',
>> +    '*incoming-fds': 'bool' } }
>>   
>>   ##
>>   # @NetdevSocketOptions:
> 


-- 
Best regards,
Vladimir