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

Commit Message

Het Gala June 9, 2022, 7:33 a.m. UTC

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 <[e-mail]>
Signed-off-by: Het Gala <[e-mail]>
---
 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(-)

Comments

Dr. David Alan Gilbert June 16, 2022, 5:26 p.m. UTC | #1

* Het Gala ([e-mail]) wrote:
> 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 <[e-mail]>
> Signed-off-by: Het Gala <[e-mail]>
> ---
>  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; \
> +})
> +
>  #endif

This looks like it should be a separate patch to me (and perhaps size_t
for len?)

> 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,

I think you mean 'if uri list 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;

Keep these as ps/pd  to make it clear which is source and dest.

> +            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)) {

I've copied in Claudio Fontana; Claudio is fighting to make snapshots
faster and has been playing with various multithread schemes for multifd
with files and fd's;  perhaps the syntax you're proposing doesn't need
to be limited to tcp.

> +                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;

'data' is an odd name; 'addresses' perhaps?

> +    uint8_t multifd_channels;
> +};
> +
> +struct OutgoingMigrateParams {
> +    struct SocketArgs *socket_args;
> +    size_t length;
> +    uint64_t total_multifd_channel;
> +} 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;

I think g_free is safe on NULL; so I think you can just do this without
the if.

> +    }
> +    if (outgoing_migrate_params.length) {

Does that ever differ from the != NULL test ?
I think you can always just set this to 0 without the test.

> +        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);
> +    }
>      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);
>  }
> 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;
> +};
>  
>  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;
>      }

Please split the HMP changes into a separate patch.

> 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

I would just say 'uri' rather than spelling it out.

> +# @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)

7.1 at the moment.

> +#
> +##
> +{ 'struct' : 'MigrateUriParameter',
> +  'data' : { 'source-uri' : 'str',
> +             'destination-uri' : 'str',
> +             '*multifd-channels' : 'uint8'} }

OK, so much higher level question - why do we specify both URIs on
each end?  Is it just the source that is used on the source side to say
which NIC to route down?  On the destination side I guess there's no
need?

Do we have some rule about needing to specify enough channels for all
the multifd channels we specify (i.e. if we specify 4 multifd channels
in the migration parameter do we have to supply 4 channels here?)
What happens with say Peter's preemption channel?

Is there some logical ordering rule; i.e. if we were to start ordering
particular multifd threads, then can we say that we allocate these
channels in the same order as this list?

>  ##
>  # @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',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:
> -- 
> 2.22.3
>

Het Gala July 13, 2022, 8:08 a.m. UTC | #2

On 16/06/22 10:56 pm, Dr. David Alan Gilbert wrote:
> * Het Gala ([e-mail]) wrote:

 > First of all, I apologise for the late reply. I was on a leave after 
internship ended

at Nutanix. Hope to learn a lot from you all in the process of 
upstreaming multifd

patches.

>> 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 <[e-mail]>
>> Signed-off-by: Het Gala <[e-mail]>
>> ---
>>   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; \
>> +})
>> +
>>   #endif
> This looks like it should be a separate patch to me (and perhaps size_t
> for len?)

 > Sure, will try to make a seperate patch for QAPI_LIST_LENGTH, and other

such utility functions from the other patches.

>
>> 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,
> I think you mean 'if uri list is provided'
 > Acknowledged.
>
>> +     * 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;
> Keep these as ps/pd  to make it clear which is source and dest.
 > Acknowledged. Will change in the upcoming patchset.
>
>> +            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)) {
> I've copied in Claudio Fontana; Claudio is fighting to make snapshots
> faster and has been playing with various multithread schemes for multifd
> with files and fd's;  perhaps the syntax you're proposing doesn't need
> to be limited to tcp.

 > For now, we are just aiming to include multifd for existing tcp 
protocol.

We would be happy to take any suggestions from Claudio Fontana and try to

include them in the upcoming patchset series.

>
>> +                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;
> 'data' is an odd name; 'addresses' perhaps?
 > Sure, Acknowledged.
>
>> +    uint8_t multifd_channels;
>> +};
>> +
>> +struct OutgoingMigrateParams {
>> +    struct SocketArgs *socket_args;
>> +    size_t length;
>> +    uint64_t total_multifd_channel;
>> +} 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;
> I think g_free is safe on NULL; so I think you can just do this without
> the if.
 > Okay, thanks for the suggestion there David.
>
>> +    }
>> +    if (outgoing_migrate_params.length) {
> Does that ever differ from the != NULL test ?
> I think you can always just set this to 0 without the test.
 > Sure.
>
>> +        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);
>> +    }
>>       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);
>>   }
>> 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;
>> +};
>>   
>>   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;
>>       }
> Please split the HMP changes into a separate patch.

 > Okay sure. Will include both on destination and source side HMP changes

into a seperate patch.

>
>> 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
> I would just say 'uri' rather than spelling it out.
 > Okay, acknowledged.
>
>> +# @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)
> 7.1 at the moment.
 > Thanks for pointing it out.
>
>> +#
>> +##
>> +{ 'struct' : 'MigrateUriParameter',
>> +  'data' : { 'source-uri' : 'str',
>> +             'destination-uri' : 'str',
>> +             '*multifd-channels' : 'uint8'} }
> OK, so much higher level question - why do we specify both URIs on
> each end?  Is it just the source that is used on the source side to say
> which NIC to route down?  On the destination side I guess there's no
> need?
>
> Do we have some rule about needing to specify enough channels for all
> the multifd channels we specify (i.e. if we specify 4 multifd channels
> in the migration parameter do we have to supply 4 channels here?)
> What happens with say Peter's preemption channel?
>
> Is there some logical ordering rule; i.e. if we were to start ordering
> particular multifd threads, then can we say that we allocate these
> channels in the same order as this list?

 > I certainly did not get your first point here David. On the 
destination side,

I think we certainly need both, destination and source uri's for making 
a connection

but on the source side, we do not require source uri, which I have not 
included

if you look at the 'Adding multi-interface support for multi-FD on 
destination

side' patch.

 > Yes, I agree with you. I will inlcude this feature in the next 
version of patchset,

where it will check the number of multifd channels coming from API and 
total

multifd channel number from qmp monitor command, and should be equal.

 > Yes David, multifd threads will be allocated in the same order, the 
user will

specify in the qmp monitor command.

>>   ##
>>   # @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',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>   
>>   ##
>>   # @migrate-incoming:
>> -- 
>> 2.22.3
>>
Regards

Het Gala

Claudio Fontana July 13, 2022, 12:54 p.m. UTC | #3

On 6/16/22 19:26, Dr. David Alan Gilbert wrote:
> * Het Gala ([e-mail]) wrote:
>> 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 <[e-mail]>
>> Signed-off-by: Het Gala <[e-mail]>
>> ---
>>  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; \
>> +})
>> +
>>  #endif
> 
> This looks like it should be a separate patch to me (and perhaps size_t
> for len?)
> 
>> 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,
> 
> I think you mean 'if uri list 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;
> 
> Keep these as ps/pd  to make it clear which is source and dest.
> 
>> +            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)) {
> 
> I've copied in Claudio Fontana; Claudio is fighting to make snapshots
> faster and has been playing with various multithread schemes for multifd
> with files and fd's;  perhaps the syntax you're proposing doesn't need
> to be limited to tcp.


Hi,

I will try to express our current problem, and see where there might be some overlap, maybe you can see more.

The current problem we are facing is, saving or restoring of VM state to disk, which in libvirt terms is "virsh save" or "virsh managedsave" and "virsh restore" or "virsh start",
is currently needlessly slow with large VMs, using upstream libvirt and qemu.

We need to get the transfer speeds of VM state (mainly RAM) to disk in the multiple GiB/s range with modern processors and NVMe disks; we have shown it is feasible.

Mainline libvirt uses QEMU migration to "fd://" to implement saving of VMs to disk, but adds copying though pipes via a libvirt "iohelper" process to work around a set of problems that are still not 100% clear to me;
(I cannot find the right email where this was discussed).

One clearly factual issue is that the QEMU migration stream is not currently O_DIRECT friendly, as it assumes it goes to network, and unfortunately for large transfers of this kind, O_DIRECT is needed due to the kernel file cache trashing problem.

So already, if your series were to address "fd://", it would potentially automatically provide an additional feature for libvirt's current save VM implementation; but I am not sure if what you are trying to achieve applies here.

Our temporary solution for libvirt to the throughput problem takes advantage of multifd migration to a "unix://" socket target to save in parallel,
with a new helper process (multifd-helper) taking the place of iohelper and performing the parallel multithreaded copy from the UNIX socket to a single file (in the latest iteration of the series),
or to multiple files in previous iterations, one for each multifd channel.

It works very well in practice, achieving dramatic throughput improvements by parallelizing the transfer reaching the GiB/s range.
This temporary solution is available here:

https://listman.redhat.com/archives/libvir-list/2022-June/232252.html

Libvirt is not accepting this approach, because the maintainer (Daniel, in Cc:) argues that the problem needs to be solved in QEMU instead,
while solving it in libvirt is an unwanted hack. My understanding is that this new feature is no more of a hack than the existing libvirt iohelper solution for basic VM save currently in mainline.

I don't really know how really this QEMU solution could look like yet.

If we code up a new QEMU "disk://" migration transport to save to a local file, and parameters to specify whether the transfer should happen in parallel, and how many parallel channels to use,
then we could solve the problem entirely in QEMU (possibly reusing some multifd code, or even not reusing that at all), but we end up with libvirt unable to efficiently put its own header as part of the savefile format libvirt expects.

An alternative could be instead to adjust the QEMU "fd://" migration protocol to add "parallel" parameters, and so keep the existing mechanism for libvirt/qemu communication for save vm,
change libvirt header read/write to be O_DIRECT friendly, and have qemu migrate in parallel directly to the open fd.

In both cases I presume that the QEMU migration stream code, including the code for all device state save, would need to be adjusted to be O_DIRECT friendly.

This modulo some additional details is my current understanding of the situation, I hope it helps.

Ciao,

Claudio

> 
>> +                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;
> 
> 'data' is an odd name; 'addresses' perhaps?
> 
>> +    uint8_t multifd_channels;
>> +};
>> +
>> +struct OutgoingMigrateParams {
>> +    struct SocketArgs *socket_args;
>> +    size_t length;
>> +    uint64_t total_multifd_channel;
>> +} 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;
> 
> I think g_free is safe on NULL; so I think you can just do this without
> the if.
> 
>> +    }
>> +    if (outgoing_migrate_params.length) {
> 
> Does that ever differ from the != NULL test ?
> I think you can always just set this to 0 without the test.
> 
>> +        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);
>> +    }
>>      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);
>>  }
>> 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;
>> +};
>>  
>>  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;
>>      }
> 
> Please split the HMP changes into a separate patch.
> 
>> 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
> 
> I would just say 'uri' rather than spelling it out.
> 
>> +# @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)
> 
> 7.1 at the moment.
> 
>> +#
>> +##
>> +{ 'struct' : 'MigrateUriParameter',
>> +  'data' : { 'source-uri' : 'str',
>> +             'destination-uri' : 'str',
>> +             '*multifd-channels' : 'uint8'} }
> 
> OK, so much higher level question - why do we specify both URIs on
> each end?  Is it just the source that is used on the source side to say
> which NIC to route down?  On the destination side I guess there's no
> need?
> 
> Do we have some rule about needing to specify enough channels for all
> the multifd channels we specify (i.e. if we specify 4 multifd channels
> in the migration parameter do we have to supply 4 channels here?)
> What happens with say Peter's preemption channel?
> 
> Is there some logical ordering rule; i.e. if we were to start ordering
> particular multifd threads, then can we say that we allocate these
> channels in the same order as this list?
> 
>>  ##
>>  # @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',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>  
>>  ##
>>  # @migrate-incoming:
>> -- 
>> 2.22.3
>>

Het Gala July 15, 2022, 8:07 a.m. UTC | #4

On 13/07/22 1:38 pm, Het Gala wrote:
>
> On 16/06/22 10:56 pm, Dr. David Alan Gilbert wrote:
>> * Het Gala ([e-mail]) wrote:
>
> > First of all, I apologise for the late reply. I was on a leave after 
> internship ended
>
> at Nutanix. Hope to learn a lot from you all in the process of 
> upstreaming multifd
>
> patches.
>
>>> 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 <[e-mail]>
>>> Signed-off-by: Het Gala <[e-mail]>
>>> ---
>>>   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; \
>>> +})
>>> +
>>>   #endif
>> This looks like it should be a separate patch to me (and perhaps size_t
>> for len?)
>
> > Sure, will try to make a seperate patch for QAPI_LIST_LENGTH, and other
>
> such utility functions from the other patches.
>
>>
>>> 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,
>> I think you mean 'if uri list is provided'
> > Acknowledged.
>>
>>> +     * 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;
>> Keep these as ps/pd  to make it clear which is source and dest.
> > Acknowledged. Will change in the upcoming patchset.
>>
>>> +            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)) {
>> I've copied in Claudio Fontana; Claudio is fighting to make snapshots
>> faster and has been playing with various multithread schemes for multifd
>> with files and fd's;  perhaps the syntax you're proposing doesn't need
>> to be limited to tcp.
>
> > For now, we are just aiming to include multifd for existing tcp 
> protocol.
>
> We would be happy to take any suggestions from Claudio Fontana and try to
>
> include them in the upcoming patchset series.
>
>>
>>> +                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;
>> 'data' is an odd name; 'addresses' perhaps?
> > Sure, Acknowledged.
>>
>>> +    uint8_t multifd_channels;
>>> +};
>>> +
>>> +struct OutgoingMigrateParams {
>>> +    struct SocketArgs *socket_args;
>>> +    size_t length;
>>> +    uint64_t total_multifd_channel;
>>> +} 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;
>> I think g_free is safe on NULL; so I think you can just do this without
>> the if.
> > Okay, thanks for the suggestion there David.
>>
>>> +    }
>>> +    if (outgoing_migrate_params.length) {
>> Does that ever differ from the != NULL test ?
>> I think you can always just set this to 0 without the test.
> > Sure.
>>
>>> +        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);
>>> +    }
>>>       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);
>>>   }
>>> 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;
>>> +};
>>>     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;
>>>       }
>> Please split the HMP changes into a separate patch.
>
> > Okay sure. Will include both on destination and source side HMP changes
>
> into a seperate patch.

 > Hi David. I am very new to upstream changes so I apologise if something

very obvious is not understandable to me. I tried to seperate HMP 
changes from

source and destination side, but the build is failing because it has 
dependencies

from qapi/migration.json and qapi/qapi-commands-migration.h files. I 
also reffered

to this commit

https://github.com/qemu/qemu/commit/abb6295b3ace5d17c3a65936913fc346616dbf14

and they have also put the QMP/HMP changes into a single commit. Let me 
know if there

is a better way we can put the HMP changes into a seperate patch.

>
>>
>>> 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
>> I would just say 'uri' rather than spelling it out.
> > Okay, acknowledged.
>>
>>> +# @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)
>> 7.1 at the moment.
> > Thanks for pointing it out.
>>
>>> +#
>>> +##
>>> +{ 'struct' : 'MigrateUriParameter',
>>> +  'data' : { 'source-uri' : 'str',
>>> +             'destination-uri' : 'str',
>>> +             '*multifd-channels' : 'uint8'} }
>> OK, so much higher level question - why do we specify both URIs on
>> each end?  Is it just the source that is used on the source side to say
>> which NIC to route down?  On the destination side I guess there's no
>> need?
>>
>> Do we have some rule about needing to specify enough channels for all
>> the multifd channels we specify (i.e. if we specify 4 multifd channels
>> in the migration parameter do we have to supply 4 channels here?)
>> What happens with say Peter's preemption channel?
>>
>> Is there some logical ordering rule; i.e. if we were to start ordering
>> particular multifd threads, then can we say that we allocate these
>> channels in the same order as this list?
>
> > I certainly did not get your first point here David. On the 
> destination side,
>
> I think we certainly need both, destination and source uri's for 
> making a connection
>
> but on the source side, we do not require source uri, which I have not 
> included
>
> if you look at the 'Adding multi-interface support for multi-FD on 
> destination
>
> side' patch.
>
> > Yes, I agree with you. I will inlcude this feature in the next 
> version of patchset,
>
> where it will check the number of multifd channels coming from API and 
> total
>
> multifd channel number from qmp monitor command, and should be equal.
>
> > Yes David, multifd threads will be allocated in the same order, the 
> user will
>
> specify in the qmp monitor command.
>
>>>   ##
>>>   # @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',
>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>     ##
>>>   # @migrate-incoming:
>>> -- 
>>> 2.22.3
>>>
> Regards
>
> Het Gala
>

Markus Armbruster July 18, 2022, 8:35 a.m. UTC | #5

Het Gala <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Het Gala <[e-mail]>
> ---
>  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.

>  #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?

> +    uint64_t total_multifd_channel;

@total_multifd_channels appears to be the sum of the
socket_args[*].multifd_channels.  Correct?

> +} 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().

>      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;

>  }
> 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.

>  
>  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.

> +#
> +##
> +{ '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.

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

Het Gala July 18, 2022, 1:33 p.m. UTC | #6

On 18/07/22 2:05 pm, Markus Armbruster wrote:
> Het Gala <[e-mail]> 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 <[e-mail]>
>> Signed-off-by: Het Gala <[e-mail]>
>> ---
>>   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.
>>   #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.
>
>> +} 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.
>>       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.
>>   }
>> 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?
>
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>   
>>   ##
>>   # @migrate-incoming:

Regards,

Het Gala

Markus Armbruster July 18, 2022, 2:33 p.m. UTC | #7

Het Gala <[e-mail]> writes:

> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>> Het Gala <[e-mail]> 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 <[e-mail]>
>>> Signed-off-by: Het Gala <[e-mail]>
>>> ---
>>>   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

Het Gala July 18, 2022, 3:17 p.m. UTC | #8

On 18/07/22 8:03 pm, Markus Armbruster wrote:
> Het Gala <[e-mail]> writes:
>
>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>> Het Gala <[e-mail]> 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 <[e-mail]>
>>>> Signed-off-by: Het Gala <[e-mail]>
>>>> ---
>>>>    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?

 > So earlier, the idea behind this was that, we had this intention to 
depreciate the migrate_multifd_channels() API from the live migration 
process. We made total_multifd_channels() function, where it used to 
calculate total number of multifd channels every time, for whichever 
function called was computation internsive so we replaced it by 
returning sum of socket_args[*].multifd_channels i.e. 
total_multifd_channel in the later patches.

  But now in the v2 patchset onwards, Thanks to inputs from Dr. David 
and Daniel, we are not depricating migrate_multifd_channels() API but 
the value from the API will be cross-referenced with sum of 
socket_args[*].multifd_channels i.e. total_multifd_channel, and error if 
they are not equal.

>
>>>> +} 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.
 > I read the comment, and it is pretty well explained out there.
>
>>>>        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 :)

 > I just wanted to have a double check on the solution you gave above 
Markus. The suggestion you have given there has been deliberately 
written in that way right, because

src_addr = socketaddress(src_uri, errp);
dst_addr = socketaddress(dst_uri, errp);
if (!src_addr) {
     return;
}
if (!dst_addr) {
     return;
}

would still be an error right according to the &err guidelines from 
error.h file.

>
>>>>    }
>>>> 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',
> ??

 > Sorry Markus, I think the statement I wrote did not make sense, I 
apologise for that. I meant to say example in the sense:

   # 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} ] } }

even this we should try to wrap within 80 character column right? or is 
that okay to go beyond 80.
>>> 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.
 > Okay thanks again Markus for your valuable suggestion. I will try to 
wrap within 75 in almost all the cases.
>>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>>   
>>>>    ##
>>>>    # @migrate-incoming:
>> Regards,
>>
>> Het Gala

Regards,

Het Gala

Markus Armbruster July 19, 2022, 7:06 a.m. UTC | #9

Het Gala <[e-mail]> writes:

> On 18/07/22 8:03 pm, Markus Armbruster wrote:
>> Het Gala <[e-mail]> writes:
>>
>>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>>> Het Gala <[e-mail]> 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 <[e-mail]>
>>>>> Signed-off-by: Het Gala <[e-mail]>
>>>>> ---

[...]

>>>>> 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?
>
> So earlier, the idea behind this was that, we had this intention to depreciate the migrate_multifd_channels() API from the live migration 
> process. We made total_multifd_channels() function, where it used to calculate total number of multifd channels every time, for whichever 
> function called was computation internsive so we replaced it by returning sum of socket_args[*].multifd_channels i.e. 
> total_multifd_channel in the later patches.
>
>  But now in the v2 patchset onwards, Thanks to inputs from Dr. David and Daniel, we are not depricating migrate_multifd_channels() API but 
> the value from the API will be cross-referenced with sum of socket_args[*].multifd_channels i.e. total_multifd_channel, and error if 
> they are not equal.

I'm afraid I don't understand.  I'm not sure I have to.  Let me loop
back to my question.

If @total_multifd_channel is always the sum of the
socket_args[*].multifd_channels, then you can always compute it on the
fly.

I.e. you can replace @total_multifd_channel by a function that returns
the sum.

Precomputing it instead is more complex, because then you need to
document that the two are the same.  Also, bug oppertunity: letting them
deviate somehow.  I figure that's worthwhile only if computing on the
fly is too expensive.

>>>>> +} 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.
>> I read the comment, and it is pretty well explained out there.
>>
>>>>>        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 :)
>
> I just wanted to have a double check on the solution you gave above Markus. The suggestion you have given there has been deliberately 
> written in that way right, because
>
> src_addr = socketaddress(src_uri, errp);
> dst_addr = socketaddress(dst_uri, errp);
> if (!src_addr) {
>     return;
> }
> if (!dst_addr) {
>     return;
> }
>
> would still be an error right according to the &err guidelines from error.h file.

Correct.

>>>>>    }

[...]

>>>>> 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',
> ??
>
> Sorry Markus, I think the statement I wrote did not make sense, I apologise for that. I meant to say example in the sense:
>
>   # 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} ] } }
>
> even this we should try to wrap within 80 character column right? or is that okay to go beyond 80.

I'd format it like

  # -> { "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} ] } }

>>>> 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.
> Okay thanks again Markus for your valuable suggestion. I will try to wrap within 75 in almost all the cases.
>>>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>>>      ##
>>>>>    # @migrate-incoming:
>>> Regards,
>>>
>>> Het Gala
>
> Regards,
>
> Het Gala

Het Gala July 19, 2022, 7:51 a.m. UTC | #10

On 19/07/22 12:36 pm, Markus Armbruster wrote:
> Het Gala <[e-mail]> writes:
>
>> On 18/07/22 8:03 pm, Markus Armbruster wrote:
>>> Het Gala <[e-mail]> writes:
>>>
>>>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>>>> Het Gala <[e-mail]> 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 <[e-mail]>
>>>>>> Signed-off-by: Het Gala <[e-mail]>
>>>>>> ---
> [...]
>
>>>>>> 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?
>> So earlier, the idea behind this was that, we had this intention to depreciate the migrate_multifd_channels() API from the live migration
>> process. We made total_multifd_channels() function, where it used to calculate total number of multifd channels every time, for whichever
>> function called was computation internsive so we replaced it by returning sum of socket_args[*].multifd_channels i.e.
>> total_multifd_channel in the later patches.
>>
>>   But now in the v2 patchset onwards, Thanks to inputs from Dr. David and Daniel, we are not depricating migrate_multifd_channels() API but
>> the value from the API will be cross-referenced with sum of socket_args[*].multifd_channels i.e. total_multifd_channel, and error if
>> they are not equal.
> I'm afraid I don't understand.  I'm not sure I have to.  Let me loop
> back to my question.
>
> If @total_multifd_channel is always the sum of the
> socket_args[*].multifd_channels, then you can always compute it on the
> fly.
>
> I.e. you can replace @total_multifd_channel by a function that returns
> the sum.
>
> Precomputing it instead is more complex, because then you need to
> document that the two are the same.  Also, bug oppertunity: letting them
> deviate somehow.  I figure that's worthwhile only if computing on the
> fly is too expensive.
 > Okay, I understand your concern. I am okay with your approach too, 
but these things are not expected to change out of qmp command context. 
So is keeping @total_multifd_channel variable should be fine? or making 
a function is better?
>>>>>> +} 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.
>>> I read the comment, and it is pretty well explained out there.
>>>
>>>>>>         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 :)
>> I just wanted to have a double check on the solution you gave above Markus. The suggestion you have given there has been deliberately
>> written in that way right, because
>>
>> src_addr = socketaddress(src_uri, errp);
>> dst_addr = socketaddress(dst_uri, errp);
>> if (!src_addr) {
>>      return;
>> }
>> if (!dst_addr) {
>>      return;
>> }
>>
>> would still be an error right according to the &err guidelines from error.h file.
> Correct.
 > Thankyou Markus.
>>>>>>     }
> [...]
>
>>>>>> 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',
>> ??
>>
>> Sorry Markus, I think the statement I wrote did not make sense, I apologise for that. I meant to say example in the sense:
>>
>>    # 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} ] } }
>>
>> even this we should try to wrap within 80 character column right? or is that okay to go beyond 80.
> I'd format it like
>
>    # -> { "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} ] } }
 > Yeah sure Markus.
>>>>> 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.
>> Okay thanks again Markus for your valuable suggestion. I will try to wrap within 75 in almost all the cases.
>>>>>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>>>>       ##
>>>>>>     # @migrate-incoming:
>>>> Regards,
>>>>
>>>> Het Gala
>> Regards,
>>
>> Het Gala

Regards,

Het Gala

Markus Armbruster July 19, 2022, 9:48 a.m. UTC | #11

Het Gala <[e-mail]> writes:

> On 19/07/22 12:36 pm, Markus Armbruster wrote:
>> Het Gala <[e-mail]> writes:
>>
>>> On 18/07/22 8:03 pm, Markus Armbruster wrote:
>>>> Het Gala <[e-mail]> writes:
>>>>
>>>>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>>>>> Het Gala <[e-mail]> 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 <[e-mail]>
>>>>>>> Signed-off-by: Het Gala <[e-mail]>
>>>>>>> ---
>> [...]
>>
>>>>>>> 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?
>>> So earlier, the idea behind this was that, we had this intention to depreciate the migrate_multifd_channels() API from the live migration
>>> process. We made total_multifd_channels() function, where it used to calculate total number of multifd channels every time, for whichever
>>> function called was computation internsive so we replaced it by returning sum of socket_args[*].multifd_channels i.e.
>>> total_multifd_channel in the later patches.
>>>
>>>   But now in the v2 patchset onwards, Thanks to inputs from Dr. David and Daniel, we are not depricating migrate_multifd_channels() API but
>>> the value from the API will be cross-referenced with sum of socket_args[*].multifd_channels i.e. total_multifd_channel, and error if
>>> they are not equal.
>> I'm afraid I don't understand.  I'm not sure I have to.  Let me loop
>> back to my question.
>>
>> If @total_multifd_channel is always the sum of the
>> socket_args[*].multifd_channels, then you can always compute it on the
>> fly.
>>
>> I.e. you can replace @total_multifd_channel by a function that returns
>> the sum.
>>
>> Precomputing it instead is more complex, because then you need to
>> document that the two are the same.  Also, bug oppertunity: letting them
>> deviate somehow.  I figure that's worthwhile only if computing on the
>> fly is too expensive.
>> Okay, I understand your concern. I am okay with your approach too, but these things are not expected to change out of qmp command context. 
>
> So is keeping @total_multifd_channel variable should be fine? or making a function is better?

I recommend making it a function unless we need a variable for
performance.

[...]

Het Gala July 19, 2022, 10:40 a.m. UTC | #12

On 19/07/22 3:18 pm, Markus Armbruster wrote:
> Het Gala <[e-mail]> writes:
>
>> On 19/07/22 12:36 pm, Markus Armbruster wrote:
>>> Het Gala <[e-mail]> writes:
>>>
>>>> On 18/07/22 8:03 pm, Markus Armbruster wrote:
>>>>> Het Gala <[e-mail]> writes:
>>>>>
>>>>>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>>>>>> Het Gala <[e-mail]> 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 <[e-mail]>
>>>>>>>> Signed-off-by: Het Gala <[e-mail]>
>>>>>>>> ---
>>> [...]
>>>
>>>>>>>> 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?
>>>> So earlier, the idea behind this was that, we had this intention to depreciate the migrate_multifd_channels() API from the live migration
>>>> process. We made total_multifd_channels() function, where it used to calculate total number of multifd channels every time, for whichever
>>>> function called was computation internsive so we replaced it by returning sum of socket_args[*].multifd_channels i.e.
>>>> total_multifd_channel in the later patches.
>>>>
>>>>    But now in the v2 patchset onwards, Thanks to inputs from Dr. David and Daniel, we are not depricating migrate_multifd_channels() API but
>>>> the value from the API will be cross-referenced with sum of socket_args[*].multifd_channels i.e. total_multifd_channel, and error if
>>>> they are not equal.
>>> I'm afraid I don't understand.  I'm not sure I have to.  Let me loop
>>> back to my question.
>>>
>>> If @total_multifd_channel is always the sum of the
>>> socket_args[*].multifd_channels, then you can always compute it on the
>>> fly.
>>>
>>> I.e. you can replace @total_multifd_channel by a function that returns
>>> the sum.
>>>
>>> Precomputing it instead is more complex, because then you need to
>>> document that the two are the same.  Also, bug oppertunity: letting them
>>> deviate somehow.  I figure that's worthwhile only if computing on the
>>> fly is too expensive.
>>> Okay, I understand your concern. I am okay with your approach too, but these things are not expected to change out of qmp command context.
>> So is keeping @total_multifd_channel variable should be fine? or making a function is better?
> I recommend making it a function unless we need a variable for
> performance.
 > Okay Markus. I will make it a function rather than a variable
>
> [...]
>

Patch

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; \
+})
+
 #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;
+    uint64_t total_multifd_channel;
+} 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);
+    }
     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);
 }
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;
+};
 
 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)
+#
+##
+{ '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',
+           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
 
 ##
 # @migrate-incoming: