Re: [PATCH 1/4] Modifying ‘migrate’ qmp command to add multi-FD socket on particular source and destination pair

[Markus Armbruster]

Het Gala <het.gala@nutanix.com> writes:

> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> i) Modified the format of the qemu monitor command : 'migrate' by adding a 
>>> list,
>>>     each element in the list consists of multi-FD connection parameters: 
>>> source
>>>     and destination uris and of the number of multi-fd channels between 
>>> each pair.
>>>
>>> ii) Information of all multi-FD connection parameters’ list, length of the 
>>> list
>>>      and total number of multi-fd channels for all the connections together 
>>> is
>>>      stored in ‘OutgoingArgs’ struct.
>>>
>>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>>   include/qapi/util.h   |  9 ++++++++
>>>   migration/migration.c | 47 ++++++++++++++++++++++++++++++--------
>>>   migration/socket.c    | 53 ++++++++++++++++++++++++++++++++++++++++---
>>>   migration/socket.h    | 17 +++++++++++++-
>>>   monitor/hmp-cmds.c    | 22 ++++++++++++++++--
>>>   qapi/migration.json   | 43 +++++++++++++++++++++++++++++++----
>>>   6 files changed, 170 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>>> index 81a2b13a33..3041feb3d9 100644
>>> --- a/include/qapi/util.h
>>> +++ b/include/qapi/util.h
>>> @@ -56,4 +56,13 @@ int parse_qapi_name(const char *name, bool complete);
>>>       (tail) = &(*(tail))->next; \
>>>   } while (0)
>>> 
>>> +#define QAPI_LIST_LENGTH(list) ({ \
>>> +    int _len = 0; \
>>> +    typeof(list) _elem; \
>>> +    for (_elem = list; _elem != NULL; _elem = _elem->next) { \
>>> +        _len++; \
>>> +    } \
>>> +    _len; \
>>> +})
>>> +
>>
>> Unless there is a compelling reason for open-coding this, make it a
>> (non-inline) function.
>>
> if kept as a function, the datatype of list is different every time
> in the qemu code, and that led to multiple copies of this function. So
> we decided, it would be best to keep it as a macro defined function. We
> would be happy to hear any suggstions to solve this problem while the
> function non-inline.

Point taken.

You could make it a function taking a GenericList *, but then you'd have
to cast actual list pointers to GenericList, which isn't nice.

>>>   #endif
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 31739b2af9..c408175aeb 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2328,13 +2328,14 @@ static bool migrate_prepare(MigrationState *s, bool 
>>> blk, bool blk_inc,
>>>       return true;
>>>   }
>>> 
>>> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>> +void qmp_migrate(const char *uri, bool has_multi_fd_uri_list,
>>> +                 MigrateUriParameterList *cap, bool has_blk, bool blk,
>>>                    bool has_inc, bool inc, bool has_detach, bool detach,
>>>                    bool has_resume, bool resume, Error **errp)
>>>   {
>>>       Error *local_err = NULL;
>>>       MigrationState *s = migrate_get_current();
>>> -    const char *p = NULL;
>>> +    const char *dst_ptr = NULL;
>>> 
>>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>>>                            has_resume && resume, errp)) {
>>> @@ -2348,20 +2349,46 @@ void qmp_migrate(const char *uri, bool has_blk, 
>>> bool blk,
>>>           }
>>>       }
>>> 
>>> +    /*
>>> +     * In case of Multi-FD migration parameters, if uri is provided,
>>> +     * supports only tcp network protocol.
>>> +     */
>>> +    if (has_multi_fd_uri_list) {
>>> +        int length = QAPI_LIST_LENGTH(cap);
>>> +        init_multifd_array(length);
>>> +        for (int i = 0; i < length; i++) {
>>> +            const char *p1 = NULL, *p2 = NULL;
>>> +            const char *multifd_dst_uri = cap->value->destination_uri;
>>> +            const char *multifd_src_uri = cap->value->source_uri;
>>> +            uint8_t multifd_channels = cap->value->multifd_channels;
>>> +            if (!strstart(multifd_dst_uri, "tcp:", &p1) ||
>>> +                !strstart(multifd_src_uri, "tcp:", &p2)) {
>>> +                error_setg(errp, "multi-fd destination and multi-fd source 
>>> "
>>> +                "uri, both should be present and follows tcp protocol 
>>> only");
>>> +                break;
>>> +            } else {
>>> +                store_multifd_migration_params(p1 ? p1 : multifd_dst_uri,
>>> +                                            p2 ? p2 : multifd_src_uri,
>>> +                                            multifd_channels, i, 
>>> &local_err);
>>> +            }
>>> +            cap = cap->next;
>>> +        }
>>> +    }
>>> +
>>>       migrate_protocol_allow_multi_channels(false);
>>> -    if (strstart(uri, "tcp:", &p) ||
>>> +    if (strstart(uri, "tcp:", &dst_ptr) ||
>>>           strstart(uri, "unix:", NULL) ||
>>>           strstart(uri, "vsock:", NULL)) {
>>>           migrate_protocol_allow_multi_channels(true);
>>> -        socket_start_outgoing_migration(s, p ? p : uri, &local_err);
>>> +        socket_start_outgoing_migration(s, dst_ptr ? dst_ptr : uri, 
>>> &local_err);
>>>   #ifdef CONFIG_RDMA
>>> -    } else if (strstart(uri, "rdma:", &p)) {
>>> -        rdma_start_outgoing_migration(s, p, &local_err);
>>> +    } else if (strstart(uri, "rdma:", &dst_ptr)) {
>>> +        rdma_start_outgoing_migration(s, dst_ptr, &local_err);
>>>   #endif
>>> -    } else if (strstart(uri, "exec:", &p)) {
>>> -        exec_start_outgoing_migration(s, p, &local_err);
>>> -    } else if (strstart(uri, "fd:", &p)) {
>>> -        fd_start_outgoing_migration(s, p, &local_err);
>>> +    } else if (strstart(uri, "exec:", &dst_ptr)) {
>>> +        exec_start_outgoing_migration(s, dst_ptr, &local_err);
>>> +    } else if (strstart(uri, "fd:", &dst_ptr)) {
>>> +        fd_start_outgoing_migration(s, dst_ptr, &local_err);
>>>       } else {
>>>           if (!(has_resume && resume)) {
>>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>> diff --git a/migration/socket.c b/migration/socket.c
>>> index 4fd5e85f50..7ca6af8cca 100644
>>> --- a/migration/socket.c
>>> +++ b/migration/socket.c
>>> @@ -32,6 +32,17 @@ struct SocketOutgoingArgs {
>>>       SocketAddress *saddr;
>>>   } outgoing_args;
>>> 
>>> +struct SocketArgs {
>>> +    struct SrcDestAddr data;
>>> +    uint8_t multifd_channels;
>>> +};
>>> +
>>> +struct OutgoingMigrateParams {
>>> +    struct SocketArgs *socket_args;
>>> +    size_t length;
>>
>> Length of what?
>
> length of the socket_args[] array. Thanks for pointing it out. I will
> be more specific for this variable in the v2 patchset series.
>
>>> +    uint64_t total_multifd_channel;
>>
>> @total_multifd_channels appears to be the sum of the
>> socket_args[*].multifd_channels.  Correct?
>
> Yes Markus, you are correct.

Sure you need to keep the sum separately?

>>> +} outgoing_migrate_params;
>>> +
>>>   void socket_send_channel_create(QIOTaskFunc f, void *data)
>>>   {
>>>       QIOChannelSocket *sioc = qio_channel_socket_new();
>>> @@ -47,6 +58,14 @@ int socket_send_channel_destroy(QIOChannel *send)
>>>           qapi_free_SocketAddress(outgoing_args.saddr);
>>>           outgoing_args.saddr = NULL;
>>>       }
>>> +
>>> +    if (outgoing_migrate_params.socket_args != NULL) {
>>> +        g_free(outgoing_migrate_params.socket_args);
>>> +        outgoing_migrate_params.socket_args = NULL;
>>> +    }
>>> +    if (outgoing_migrate_params.length) {
>>> +        outgoing_migrate_params.length = 0;
>>> +    }
>>>       return 0;
>>>   }
>>> 
>>> @@ -117,13 +136,41 @@ 
>>> socket_start_outgoing_migration_internal(MigrationState *s,
>>>   }
>>> 
>>>   void socket_start_outgoing_migration(MigrationState *s,
>>> -                                     const char *str,
>>> +                                     const char *dst_str,
>>>                                        Error **errp)
>>>   {
>>>       Error *err = NULL;
>>> -    SocketAddress *saddr = socket_parse(str, &err);
>>> +    SocketAddress *dst_saddr = socket_parse(dst_str, &err);
>>> +    if (!err) {
>>> +        socket_start_outgoing_migration_internal(s, dst_saddr, &err);
>>> +    }
>>> +    error_propagate(errp, err);
>>> +}
>>> +
>>> +void init_multifd_array(int length)
>>> +{
>>> +    outgoing_migrate_params.socket_args = g_new0(struct SocketArgs, 
>>> length);
>>> +    outgoing_migrate_params.length = length;
>>> +    outgoing_migrate_params.total_multifd_channel = 0;
>>> +}
>>> +
>>> +void store_multifd_migration_params(const char *dst_uri,
>>> +                                    const char *src_uri,
>>> +                                    uint8_t multifd_channels,
>>> +                                    int idx, Error **errp)
>>> +{
>>> +    Error *err = NULL;
>>> +    SocketAddress *src_addr = NULL;
>>> +    SocketAddress *dst_addr = socket_parse(dst_uri, &err);
>>> +    if (src_uri) {
>>> +        src_addr = socket_parse(src_uri, &err);
>>> +    }
>>
>> Incorrect use of &err.  error.h's big comment:
>>
>>   * Receive and accumulate multiple errors (first one wins):
>>   *     Error *err = NULL, *local_err = NULL;
>>   *     foo(arg, &err);
>>   *     bar(arg, &local_err);
>>   *     error_propagate(&err, local_err);
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>   *
>>   * Do *not* "optimize" this to
>>   *     Error *err = NULL;
>>   *     foo(arg, &err);
>>   *     bar(arg, &err); // WRONG!
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>   * because this may pass a non-null err to bar().
>>
> Thankyou Markus for sharing this knowledge. I was unaware of the
> dont's with &err.

The big comment should help you along.  If it doesn't, just ask.

>>>       if (!err) {
>>> -        socket_start_outgoing_migration_internal(s, saddr, &err);
>>> +        outgoing_migrate_params.socket_args[idx].data.dst_addr = dst_addr;
>>> +        outgoing_migrate_params.socket_args[idx].data.src_addr = src_addr;
>>> +        outgoing_migrate_params.socket_args[idx].multifd_channels
>>> +                                                         = 
>>> multifd_channels;
>>> +        outgoing_migrate_params.total_multifd_channel += multifd_channels;
>>>       }
>>>       error_propagate(errp, err);
>>
>> Consider
>>
>>         struct SocketArgs *sa = &outgoing_migrate_params.socket_args[idx];
>>         SocketAddress *src_addr, *dst_addr;
>>
>>         src_addr = socketaddress(src_uri, errp);
>>         if (!src_addr) {
>>             return;
>>         }
>>
>>         dst_addr = socketaddress(dst_uri, errp);
>>         if (!dst_addr) {
>>             return;
>>         }
>>
>>         sa->data.dst_addr = dst_addr;
>>         sa->data.src_addr = src_addr;
>>         sa->multifd_channels = multifd_channels;
>>         outgoing_migrate_params.total_multifd_channel += multifd_channels;
>
> Thanks Markus for this amazing suggestion. Your approach looks
> simpler to understand and also resolves the error it had with &err. I
> will surely implement this in the upcoming v2 patchset.

You're welcome :)

>>>   }
>>> diff --git a/migration/socket.h b/migration/socket.h
>>> index 891dbccceb..bba7f177fe 100644
>>> --- a/migration/socket.h
>>> +++ b/migration/socket.h
>>> @@ -19,12 +19,27 @@
>>> 
>>>   #include "io/channel.h"
>>>   #include "io/task.h"
>>> +#include "migration.h"
>>> +
>>> +/* info regarding destination and source uri */
>>> +struct SrcDestAddr {
>>> +    SocketAddress *dst_addr;
>>> +    SocketAddress *src_addr;
>>> +};
>>
>> QEMU coding style wants a typedef.
>
> Okay, acknowledged.
>
>>> 
>>>   void socket_send_channel_create(QIOTaskFunc f, void *data);
>>>   int socket_send_channel_destroy(QIOChannel *send);
>>> 
>>>   void socket_start_incoming_migration(const char *str, Error **errp);
>>> 
>>> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
>>> +void socket_start_outgoing_migration(MigrationState *s, const char 
>>> *dst_str,
>>>                                        Error **errp);
>>> +
>>> +int multifd_list_length(MigrateUriParameterList *list);
>>> +
>>> +void init_multifd_array(int length);
>>> +
>>> +void store_multifd_migration_params(const char *dst_uri, const char 
>>> *src_uri,
>>> +                                    uint8_t multifd_channels, int idx,
>>> +                                    Error **erp);
>>>   #endif
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 622c783c32..2db539016a 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -56,6 +56,9 @@
>>>   #include "migration/snapshot.h"
>>>   #include "migration/misc.h"
>>> 
>>> +/* Default number of multi-fd channels */
>>> +#define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
>>> +
>>>   #ifdef CONFIG_SPICE
>>>   #include <spice/enums.h>
>>>   #endif
>>> @@ -1574,10 +1577,25 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>>       bool inc = qdict_get_try_bool(qdict, "inc", false);
>>>       bool resume = qdict_get_try_bool(qdict, "resume", false);
>>>       const char *uri = qdict_get_str(qdict, "uri");
>>> +
>>> +    const char *src_uri = qdict_get_str(qdict, "source-uri");
>>> +    const char *dst_uri = qdict_get_str(qdict, "destination-uri");
>>> +    uint8_t multifd_channels = qdict_get_try_int(qdict, "multifd-channels",
>>> +                                        DEFAULT_MIGRATE_MULTIFD_CHANNELS);
>>>       Error *err = NULL;
>>> +    MigrateUriParameterList *caps = NULL;
>>> +    MigrateUriParameter *value;
>>> +
>>> +    value = g_malloc0(sizeof(*value));
>>> +    value->source_uri = (char *)src_uri;
>>> +    value->destination_uri = (char *)dst_uri;
>>> +    value->multifd_channels = multifd_channels;
>>> +    QAPI_LIST_PREPEND(caps, value);
>>> +
>>> +    qmp_migrate(uri, !!caps, caps, !!blk, blk, !!inc,
>>> +                inc, false, false, true, resume, &err);
>>> +    qapi_free_MigrateUriParameterList(caps);
>>> 
>>> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>> -                false, false, true, resume, &err);
>>>       if (hmp_handle_error(mon, err)) {
>>>           return;
>>>       }
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 6130cd9fae..fb259d626b 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1454,12 +1454,38 @@
>>>   ##
>>>   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>> 
>>> +##
>>> +# @MigrateUriParameter:
>>> +#
>>> +# Information regarding which source interface is connected to which
>>> +# destination interface and number of multifd channels over each interface.
>>> +#
>>> +# @source-uri: the Uniform Resource Identifier of the source VM.
>>> +#              Default port number is 0.
>>> +#
>>> +# @destination-uri: the Uniform Resource Identifier of the destination VM
>>> +#
>>> +# @multifd-channels: number of parallel multifd channels used to migrate 
>>> data
>>> +#                    for specific source-uri and destination-uri. Default 
>>> value
>>> +#                    in this case is 2 (Since 4.0)
>>
>> You mean "(Since 7.1)", I guess.
>
> Yes yes. Also David pointed this thing out. I will update the version
> in the v2 patchset.
>
>>> +#
>>> +##
>>> +{ 'struct' : 'MigrateUriParameter',
>>> +  'data' : { 'source-uri' : 'str',
>>> +             'destination-uri' : 'str',
>>> +             '*multifd-channels' : 'uint8'} }
>>> +
>>>   ##
>>>   # @migrate:
>>>   #
>>>   # Migrates the current running guest to another Virtual Machine.
>>>   #
>>>   # @uri: the Uniform Resource Identifier of the destination VM
>>> +#       for migration thread
>>> +#
>>> +# @multi-fd-uri-list: list of pair of source and destination VM Uniform
>>> +#                     Resource Identifiers with number of multifd-channels
>>> +#                     for each pair
>>>   #
>>>   # @blk: do block migration (full disk copy)
>>>   #
>>> @@ -1479,20 +1505,27 @@
>>>   # 1. The 'query-migrate' command should be used to check migration's 
>>> progress
>>>   #    and final result (this information is provided by the 'status' 
>>> member)
>>>   #
>>> -# 2. All boolean arguments default to false
>>> +# 2. The uri argument should have the Uniform Resource Identifier of 
>>> default
>>> +#    destination VM. This connection will be bound to default network
>>> +#
>>> +# 3. All boolean arguments default to false
>>>   #
>>> -# 3. The user Monitor's "detach" argument is invalid in QMP and should not
>>> +# 4. The user Monitor's "detach" argument is invalid in QMP and should not
>>>   #    be used
>>>   #
>>>   # Example:
>>>   #
>>> -# -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>> +# -> { "execute": "migrate",
>>> +#                 "arguments": { "uri": "tcp:0:4446", "multi-fd-uri-list": 
>>> [ {
>>> +#                                "source-uri": "tcp::6900", 
>>> "destination-uri": "tcp:0:4480",
>>> +#                                "multifd-channels": 4}, { "source-uri": 
>>> "tcp:10.0.0.0: ",
>>> +#                                "destination-uri": "tcp:11.0.0.0:7789", 
>>> "multifd-channels": 5} ] } }
>>>   # <- { "return": {} }
>>>   #
>>>   ##
>>>   { 'command': 'migrate',
>>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>>> -           '*detach': 'bool', '*resume': 'bool' } }
>>> +  'data': {'uri': 'str', '*multi-fd-uri-list': ['MigrateUriParameter'], 
>>> '*blk': 'bool',
??
>> Long line.
>
> Okay, acknowledged. Even for example, should it be under 80
> characters per line, or that is fine?

docs/devel/style.rst:

    Line width
    ==========

    Lines should be 80 characters; try not to make them longer.

    Sometimes it is hard to do, especially when dealing with QEMU subsystems
    that use long function or symbol names. If wrapping the line at 80 columns
    is obviously less readable and more awkward, prefer not to wrap it; better
    to have an 85 character line than one which is awkwardly wrapped.

    Even in that case, try not to make lines much longer than 80 characters.
    (The checkpatch script will warn at 100 characters, but this is intended
    as a guard against obviously-overlength lines, not a target.)

Personally, I very much prefer to wrap between 70 and 75 except where it
impairs legibility.

>>
>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>  
>>>   ##
>>>   # @migrate-incoming:
>
> Regards,
>
> Het Gala