Re: [PATCH v2 5/8] net/tap: implement interfaces for local migration

[Steven Sistare <steven.sistare@oracle.com>]

On 9/5/2025 6:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 04.09.25 10:41, Vladimir Sementsov-Ogievskiy wrote:
>> On 03.09.25 19:09, Steven Sistare wrote:
>>> On 9/3/2025 11:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 03.09.25 17:34, Daniel P. Berrangé wrote:
>>>>> On Wed, Sep 03, 2025 at 04:37:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Handle local-incoming option:
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>>>> ---
>>>>>>   include/net/tap.h |   4 ++
>>>>>>   net/tap.c         | 136 +++++++++++++++++++++++++++++++++++++++-------
>>>>>>   2 files changed, 119 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/tap.h b/include/net/tap.h
>>>>>> index 6f34f13eae..3ef2e2dbae 100644
>>>>>> --- a/include/net/tap.h
>>>>>> +++ b/include/net/tap.h
>>>>>> @@ -30,7 +30,11 @@
>>>>>>   int tap_enable(NetClientState *nc);
>>>>>>   int tap_disable(NetClientState *nc);
>>>>>> +bool tap_local_incoming(NetClientState *nc);
>>>>>>   int tap_get_fd(NetClientState *nc);
>>>>>> +int tap_load(NetClientState *nc, QEMUFile *f);
>>>>>> +int tap_save(NetClientState *nc, QEMUFile *f);
>>>>>> +
>>>>>>   #endif /* QEMU_NET_TAP_H */
>>>>>> diff --git a/net/tap.c b/net/tap.c
>>>>>> index a9d955ac5f..499db756ea 100644
>>>>>> --- a/net/tap.c
>>>>>> +++ b/net/tap.c
>>>>>> @@ -35,6 +35,8 @@
>>>>>>   #include "net/eth.h"
>>>>>>   #include "net/net.h"
>>>>>>   #include "clients.h"
>>>>>> +#include "migration/migration.h"
>>>>>> +#include "migration/qemu-file.h"
>>>>>>   #include "monitor/monitor.h"
>>>>>>   #include "system/system.h"
>>>>>>   #include "qapi/error.h"
>>>>>> @@ -82,6 +84,7 @@ typedef struct TAPState {
>>>>>>       VHostNetState *vhost_net;
>>>>>>       unsigned host_vnet_hdr_len;
>>>>>>       Notifier exit;
>>>>>> +    bool local_incoming;
>>>>>>   } TAPState;
>>>>>>   static void launch_script(const char *setup_script, const char *ifname,
>>>>>> @@ -803,6 +806,40 @@ static int net_tap_init_vhost(TAPState *s, Error **errp) {
>>>>>>       return 0;
>>>>>>   }
>>>>>> +int tap_save(NetClientState *nc, QEMUFile *f)
>>>>>> +{
>>>>>> +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
>>>>>> +
>>>>>> +    qemu_file_put_fd(f, s->fd);
>>>>>> +    qemu_put_byte(f, s->using_vnet_hdr);
>>>>>> +    qemu_put_byte(f, s->has_ufo);
>>>>>> +    qemu_put_byte(f, s->has_uso);
>>>>>> +    qemu_put_byte(f, s->enabled);
>>>>>> +    qemu_put_be32(f, s->host_vnet_hdr_len);
>>>>>
>>>>>
>>>>> Is it neccessary to transfer that metadata, or is there perhaps a way
>>>>> for the other side to query the TAP FD configuration from the kernel
>>>>> to detect this ?
>>>>
>>>> Oh, good question, thanks for it. I just added everything and then I was debugging other places.
>>>>
>>>> for hdr_len we have TUNGETVNETHDRSZ, so it's possible.
>>>>
>>>> using_vnet_hdr, seems is equal to initial vnet_hdr option (with default to 1 if not specified), will doublecheck
>>>>
>>>> for ufo/uso, which are set through TUNSETOFFLOAD, we don't have direct way to
>>>> get the state. But we can use the fact, that qemu tries to set them once,
>>>> and these variables are unchanged after initialization. So we can try set
>>>> same flags on target the same way, to understand what we have. Still,
>>>> this doesn't seem absolutely safe.. Kernel may behave differently than
>>>> for previous initialization, probably due to some changed settings.
>>>>
>>>> for enabled it seems not possible, but we handle it in virtio layer.. Oops,
>>>> probably I always migrate enabled=false with this code, will check.
>>>>
>>>> ---
>>>>
>>>> On the other hand, calling extra ioctls to learn something lead to extra downtime
>>>> (should be measured to be a good argument).
>>>>
>>>> Also, just architecturally: seems better not ask third agent about metadata that we already know.
>>>>
>>>> ---
>>>>
>>>> About forward-compatibility (if we add new fields here) - agree.
>>>>
>>>> Maybe turn several boolean fields into one flags field. This way we'll get several "reserved" bits for future changes.
>>>>
>>>>>
>>>>> I'm concerned that this code / wire format is not extensible if we ever
>>>>> add another similar field to TAPState in the future.
>>>
>>> tap_save and tap_load should be replaced with a VMStateDescription for future
>>> extensibility.  Use VMSTATE_FD for the fd.  Define a postload hook for
>>> tap_read_poll and net_tap_init_vhost.
>>
>> How it works? I thought, if I add new field to vmsd, destination will try to load it anyway (as it loads them in a loop in vmstate_load_state()).. So, we'll have to add same new capabilities anyway to "enable" new fields (with help of .field_exists)? Same way we can add new field to current realization, with new migration capability and "if" in _load() function..
>>
>> Still, seems using VMSD is better anyway, so I should do it.
> 
> answering myself: at least, fields has versioning feature, which is intended for future-compatibility.

Plus you can add new fields compatibly by adding a subsection.
See Subsections in docs/devel/migration/main.rst.

- steve