* [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd
@ 2022-07-21 19:56 Het Gala
2022-07-21 19:56 ` [PATCH v2 1/7] multifd: adding more helper functions in util files for live migration Het Gala
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala
This is v2 of multiple interface support over multifd for live migration.
Links of previous versions:
v1:
https://lore.kernel.org/qemu-devel/20220609073305.142515-1-het.gala@nutanix.com
Thanks to David, Daniel and Markus for their valuable insights.
v1->v2 changelog:
- little helper functions shifted/added inside util* file into a separate patch
- HMP changes are split into separate patch
- migrate_multifd_channel() API been restored
- removed total_multifd_channel variable 'OutgoingMigrateParams' struct.
- qio_channel_socket_connect_async and qio_channel_socket_connect_sync methods
are not updated
- instead introduced variants qio_channel_socket_connect_full_async and
qio_channel_socket_connect_full_sync for connecting particular src and dest
ips for live migration
- added code for validation for mismatch of IPv4 vs IPv6 for src and dest addr.
- Few nit whitespace changes in qemu-sockets, are split into separate patch.
Abstract:
========
As of now, the multi-FD feature supports connection over the default network
only. This Patchset series is a Qemu side implementation of providing multiple
interfaces support for multi-FD. This enables us to fully utilize dedicated or
multiple NICs in case bonding of NICs is not possible.
What's new
==========
The HMP code changes for source and destination side are split into a separate
patch.
Little helper functions for live migration from different patches have
been added together into a single separate patch.
migrate_multifd_channels existing API have been restored. Instead of
pre-computing total_multifd_channels parameter and storing it in the struct,
it is now calculted on the fly to reduce complexity, and are equal the number
of multifd channels coming from live migration API and qmp monitor command,
are verified.
Instead of breaking existing methods while establishing connection between
any non-default src and dest, created newer methods for accomodating src_addr
as extra parameter for establishing connection.
Added a check for validating no mismatch between src and dest ips for IPv4 vs
IPv6 happens before connection is created.
----------------
Het Gala (7):
multifd: adding more helper functions in util files for live migration
multifd: modifying 'migrate' qmp command to add multifd socket on
particular src and dest pair
multifd: adding multi-interface support for multifd on destination
side
multifd: HMP changes for multifd source and destination side
multifd: establishing connection between any non-default src and dest
pair
muitlfd: Correcting nit : whitespace error changes in qemu-sockets.c
file
multifd: adding support for multifd connections dynamically
include/io/channel-socket.h | 44 ++++++++
include/qapi/util.h | 12 ++
include/qemu/sockets.h | 6 +-
io/channel-socket.c | 93 ++++++++++++----
migration/migration.c | 195 ++++++++++++++++++++++++++++-----
migration/migration.h | 2 +
migration/multifd.c | 6 +-
migration/socket.c | 106 ++++++++++++++----
migration/socket.h | 25 ++++-
monitor/hmp-cmds.c | 66 +++++------
qapi/migration.json | 93 ++++++++++++++--
qapi/qapi-util.c | 36 ++++++
qemu-options.hx | 18 +++
softmmu/vl.c | 30 ++++-
tests/unit/test-util-sockets.c | 16 +--
util/qemu-sockets.c | 112 +++++++++++++------
16 files changed, 692 insertions(+), 168 deletions(-)
--
2.22.3
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/7] multifd: adding more helper functions in util files for live migration
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-21 19:56 ` [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair Het Gala
` (5 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) strList_from_string() shifted from hm-cmds.c -> qapi-util.c
ii) Adding qemu_string_count_delim() helper func. in qapi-util.c and
QAPI_LIST_LENGTH() macro defined func. in util.h
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
include/qapi/util.h | 12 ++++++++++++
monitor/hmp-cmds.c | 31 +++----------------------------
qapi/qapi-util.c | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/include/qapi/util.h b/include/qapi/util.h
index 81a2b13a33..36164ca7ba 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -29,6 +29,8 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
Error **errp);
int parse_qapi_name(const char *name, bool complete);
+struct strList *strList_from_string(const char *in, char c);
+int qemu_string_count_delim(const char *str, char delim);
/*
* For any GenericList @list, insert @element at the front.
@@ -56,4 +58,14 @@ int parse_qapi_name(const char *name, bool complete);
(tail) = &(*(tail))->next; \
} while (0)
+/* provides the length of any type of list */
+#define QAPI_LIST_LENGTH(list) ({ \
+ size_t _len = 0; \
+ typeof(list) _elem; \
+ for (_elem = list; _elem != NULL; _elem = _elem->next) { \
+ _len++; \
+ } \
+ _len; \
+})
+
#endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6dc79e0d5..6bb6424215 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -43,6 +43,7 @@
#include "qapi/qapi-commands-stats.h"
#include "qapi/qapi-commands-tpm.h"
#include "qapi/qapi-commands-ui.h"
+#include "qapi/util.h"
#include "qapi/qapi-visit-net.h"
#include "qapi/qapi-visit-migration.h"
#include "qapi/qmp/qdict.h"
@@ -71,32 +72,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
return false;
}
-/*
- * Produce a strList from a comma separated list.
- * A NULL or empty input string return NULL.
- */
-static strList *strList_from_comma_list(const char *in)
-{
- strList *res = NULL;
- strList **tail = &res;
-
- while (in && in[0]) {
- char *comma = strchr(in, ',');
- char *value;
-
- if (comma) {
- value = g_strndup(in, comma - in);
- in = comma + 1; /* skip the , */
- } else {
- value = g_strdup(in);
- in = NULL;
- }
- QAPI_LIST_APPEND(tail, value);
- }
-
- return res;
-}
-
void hmp_info_name(Monitor *mon, const QDict *qdict)
{
NameInfo *info;
@@ -1121,7 +1096,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
migrate_announce_params());
qapi_free_strList(params->interfaces);
- params->interfaces = strList_from_comma_list(interfaces_str);
+ params->interfaces = strList_from_string(interfaces_str, ',');
params->has_interfaces = params->interfaces != NULL;
params->id = g_strdup(id);
params->has_id = !!params->id;
@@ -2399,7 +2374,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
request->provider = provider_idx;
if (names && !g_str_equal(names, "*")) {
request->has_names = true;
- request->names = strList_from_comma_list(names);
+ request->names = strList_from_string(names, ',');
}
QAPI_LIST_PREPEND(request_list, request);
}
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 63596e11c5..9672ac6018 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -152,3 +152,38 @@ int parse_qapi_name(const char *str, bool complete)
}
return p - str;
}
+
+/*
+ * Produce a strList from a delimiter separated list.
+ * A NULL or empty input string return NULL.
+ */
+strList *strList_from_string(const char *in, char c)
+{
+ strList *res = NULL;
+ strList **tail = &res;
+
+ while (in && in[0]) {
+ char *ch = strchr(in, c);
+ char *value;
+
+ if (ch) {
+ value = g_strndup(in, ch - in);
+ in = ch + 1; /* skip the , */
+ } else {
+ value = g_strdup(in);
+ in = NULL;
+ }
+ QAPI_LIST_APPEND(tail, value);
+ }
+
+ return res;
+}
+
+int qemu_string_count_delim(const char *str, char delim)
+{
+ int count = 0;
+ for (int i = 0; i < strlen(str); i++) {
+ count += (str[i] == delim);
+ }
+ return count;
+}
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
2022-07-21 19:56 ` [PATCH v2 1/7] multifd: adding more helper functions in util files for live migration Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-26 11:13 ` Daniel P. Berrangé
2022-07-21 19:56 ` [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side Het Gala
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
each element in the list consisting of multifd connection parameters: source
uri, destination uri and of the number of multifd channels between each pair.
ii) Information of all multifd connection parameters' list and length of the
list is stored in 'OutgoingMigrateParams' struct.
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 52 +++++++++++++++++++++++++++++--------
migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
migration/socket.h | 19 +++++++++++++-
monitor/hmp-cmds.c | 1 +
qapi/migration.json | 47 +++++++++++++++++++++++++++++----
5 files changed, 160 insertions(+), 19 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index e03f698a3c..572b909423 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2380,13 +2380,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)) {
@@ -2400,20 +2401,51 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
}
+ /*
+ * In case of Multi-FD migration, source and destination uri
+ * 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 *pd = NULL, *ps = 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:", &pd) ||
+ !strstart(multifd_src_uri, "tcp:", &ps)) {
+ error_setg(errp, "multi-fd destination and multi-fd source "
+ "uri, both should be present and follows tcp protocol only");
+ return;
+ } else {
+ store_multifd_migration_params(pd ? pd : multifd_dst_uri,
+ ps ? ps : multifd_src_uri,
+ multifd_channels, i, &local_err);
+ }
+ cap = cap->next;
+ }
+
+ if (outgoing_param_total_multifds() != migrate_multifd_channels()) {
+ error_setg(errp, "Total multifd channel number mismatch");
+ return;
+ }
+ }
+
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 e6fdf3c5e1..f199430bc2 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -32,6 +32,28 @@ struct SocketOutgoingArgs {
SocketAddress *saddr;
} outgoing_args;
+struct SocketArgs {
+ struct SrcDestAddr address;
+ uint8_t multifd_channels;
+};
+
+struct OutgoingMigrateParams {
+ struct SocketArgs *socket_args;
+ size_t socket_args_arr_len;
+} outgoing_migrate_params;
+
+int outgoing_param_total_multifds(void)
+{
+ size_t len = outgoing_migrate_params.socket_args_arr_len;
+ uint64_t total_multifd_channels = 0;
+
+ for (int i = 0; i < len; i++) {
+ total_multifd_channels +=
+ outgoing_migrate_params.socket_args[i].multifd_channels;
+ }
+ return total_multifd_channels;
+}
+
void socket_send_channel_create(QIOTaskFunc f, void *data)
{
QIOChannelSocket *sioc = qio_channel_socket_new();
@@ -65,6 +87,9 @@ int socket_send_channel_destroy(QIOChannel *send)
qapi_free_SocketAddress(outgoing_args.saddr);
outgoing_args.saddr = NULL;
}
+ g_free(outgoing_migrate_params.socket_args);
+ outgoing_migrate_params.socket_args = NULL;
+ outgoing_migrate_params.socket_args_arr_len = 0;
return 0;
}
@@ -135,17 +160,46 @@ 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, saddr, &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.socket_args_arr_len = length;
+}
+
+void store_multifd_migration_params(const char *dst_uri,
+ const char *src_uri,
+ uint8_t multifd_channels,
+ int idx, Error **errp)
+{
+ struct SocketArgs *sa = &outgoing_migrate_params.socket_args[idx];
+ SocketAddress *src_addr, *dst_addr;
+
+ src_addr = socket_parse(src_uri, errp);
+ if (!src_addr) {
+ return;
+ }
+
+ dst_addr = socket_parse(dst_uri, errp);
+ if (!dst_addr) {
+ return;
+ }
+
+ sa->address.dst_addr = dst_addr;
+ sa->address.src_addr = src_addr;
+ sa->multifd_channels = multifd_channels;
+}
+
static void socket_accept_incoming_migration(QIONetListener *listener,
QIOChannelSocket *cioc,
gpointer opaque)
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..0cbb7220ac 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,13 +19,30 @@
#include "io/channel.h"
#include "io/task.h"
+#include "migration.h"
+/* info regarding destination and source uri */
+typedef struct SrcDestAddr {
+ SocketAddress *dst_addr;
+ SocketAddress *src_addr;
+} SrcDestAddr;
+
+
+int outgoing_param_total_multifds(void);
void socket_send_channel_create(QIOTaskFunc f, void *data);
QIOChannel *socket_send_channel_create_sync(Error **errp);
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 6bb6424215..8d25fee4be 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -59,6 +59,7 @@
#include "migration/snapshot.h"
#include "migration/misc.h"
+
#ifdef CONFIG_SPICE
#include <spice/enums.h>
#endif
diff --git a/qapi/migration.json b/qapi/migration.json
index 81185d4311..456247af8f 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1449,12 +1449,37 @@
##
{ '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: uri of the source VM. Default port number is 0.
+#
+# @destination-uri: uri 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 7.1)
+#
+##
+{ '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)
#
@@ -1474,20 +1499,32 @@
# 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. The user Monitor's "detach" argument is invalid in QMP and should not
+# 3. All boolean arguments default to false
+#
+# 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
2022-07-21 19:56 ` [PATCH v2 1/7] multifd: adding more helper functions in util files for live migration Het Gala
2022-07-21 19:56 ` [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-26 11:20 ` Daniel P. Berrangé
2022-07-21 19:56 ` [PATCH v2 4/7] multifd: HMP changes for multifd source and " Het Gala
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) Modified the format of qemu monitor command: 'migrate-incoming' by adding
a list, each element in the list to open socket listeners with a given
number of multifd channels.
ii) Qemu starts with -incoming flag defer and -multi-fd-incoming defer to
allow the modified 'migrate-incoming' command to be used.
iii) Format for -multi-fd-incoming flag as a comma separated string has been
added with each substring containing listener socket address and number
of sockets to open.
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 143 ++++++++++++++++++++++++++++++++++++------
migration/migration.h | 2 +
migration/socket.c | 11 ++--
migration/socket.h | 3 +-
qapi/migration.json | 46 ++++++++++++--
qapi/qapi-util.c | 3 +-
qemu-options.hx | 18 ++++++
softmmu/vl.c | 30 ++++++++-
8 files changed, 221 insertions(+), 35 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 572b909423..c58b81576c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -486,28 +486,41 @@ void migrate_add_address(SocketAddress *address)
QAPI_CLONE(SocketAddress, address));
}
-static void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri,
+ uint8_t multifd_count, int idx,
+ Error **errp)
{
const char *p = NULL;
- migrate_protocol_allow_multi_channels(false); /* reset it anyway */
- qapi_event_send_migration(MIGRATION_STATUS_SETUP);
- if (strstart(uri, "tcp:", &p) ||
- strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL)) {
- migrate_protocol_allow_multi_channels(true);
- socket_start_incoming_migration(p ? p : uri, errp);
-#ifdef CONFIG_RDMA
- } else if (strstart(uri, "rdma:", &p)) {
- rdma_start_incoming_migration(p, errp);
-#endif
- } else if (strstart(uri, "exec:", &p)) {
- exec_start_incoming_migration(p, errp);
- } else if (strstart(uri, "fd:", &p)) {
- fd_start_incoming_migration(p, errp);
+ if (multifd_count == 0) {
+ migrate_protocol_allow_multi_channels(false); /* reset it anyway */
+ qapi_event_send_migration(MIGRATION_STATUS_SETUP);
+ if (strstart(uri, "tcp:", &p) ||
+ strstart(uri, "unix:", NULL) ||
+ strstart(uri, "vsock:", NULL)) {
+ migrate_protocol_allow_multi_channels(true);
+ #ifdef CONFIG_RDMA
+ } else if (strstart(uri, "rdma:", &p)) {
+ rdma_start_incoming_migration(p, errp);
+ #endif
+ } else if (strstart(uri, "exec:", &p)) {
+ exec_start_incoming_migration(p, errp);
+ } else if (strstart(uri, "fd:", &p)) {
+ fd_start_incoming_migration(p, errp);
+ } else {
+ error_setg(errp, "unknown migration protocol: %s", uri);
+ }
} else {
- error_setg(errp, "unknown migration protocol: %s", uri);
+ /* multi-FD parameters only support tcp network protocols */
+ if (!strstart(uri, "tcp:", &p)) {
+ error_setg(errp, "multifd-destination uri supports "
+ "tcp protocol only");
+ return;
+ }
+ store_multifd_migration_params(p ? p : uri, NULL, multifd_count,
+ idx, errp);
}
+ socket_start_incoming_migration(p ? p : uri, multifd_count, errp);
}
static void process_incoming_migration_bh(void *opaque)
@@ -2192,7 +2205,8 @@ void migrate_del_blocker(Error *reason)
migration_blockers = g_slist_remove(migration_blockers, reason);
}
-void qmp_migrate_incoming(const char *uri, Error **errp)
+/* migrate_incoming function is for -incoming flag process */
+void migrate_incoming(const char *uri, Error **errp)
{
Error *local_err = NULL;
static bool once = true;
@@ -2210,7 +2224,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
return;
}
- qemu_start_incoming_migration(uri, &local_err);
+ qemu_start_incoming_migration(uri, 0, 0, &local_err);
if (local_err) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -2221,6 +2235,95 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
once = false;
}
+/*
+ * multi_fd_migrate_incoming function is for -multi-fd-migrate-incoming
+ * flag process
+ */
+void multi_fd_migrate_incoming(const char *uri, Error **errp)
+{
+ Error *local_err = NULL;
+ static bool once = true;
+
+ if (!once) {
+ error_setg(errp, "The incoming migration has already been started");
+ return;
+ }
+ if (!runstate_check(RUN_STATE_INMIGRATE)) {
+ error_setg(errp, "'-multi-fd-incoming' was not specified on the command line");
+ return;
+ }
+
+ strList *st = strList_from_string(uri, ',');
+ strList *r = st;
+ int length = QAPI_LIST_LENGTH(st);
+ init_multifd_array(length);
+
+ for (int i = 0; i < length; i++) {
+ const char *uri = NULL, *ret = NULL;
+ const char *str = r->value;
+ uint8_t multifd_channels = DEFAULT_MIGRATE_MULTIFD_CHANNELS;
+ int parse_count = qemu_string_count_delim(str, ':');
+ if (parse_count < 2 || parse_count > 3) {
+ error_setg(errp, "Invalid format of string-id %d in "
+ "'-multi-fd-incoming' flag", i);
+ return;
+ }
+ if (parse_count == 3) {
+ ret = strrchr(str, ':');
+ uri = g_strndup(str, strlen(str) - strlen(ret));
+ multifd_channels = atoi(ret + 1);
+ }
+ qemu_start_incoming_migration(parse_count == 2 ? str : uri,
+ multifd_channels, i, &local_err);
+ r = r->next;
+ }
+ if (local_err) {
+ yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ once = false;
+}
+
+/* qmp_migrate_incoming comes from qemu qmp monitor command */
+void qmp_migrate_incoming(const char *uri, bool has_multi_fd_uri_list,
+ MigrateIncomingUriList *cap, Error **errp)
+{
+ Error *local_err = NULL;
+ static bool once = true;
+
+ if (!once) {
+ error_setg(errp, "The incoming migration has already been started");
+ return;
+ }
+
+ if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+ return;
+ }
+
+ /* For migration thread */
+ qemu_start_incoming_migration(uri, 0, 0, &local_err);
+
+ /* For Multi-FD */
+ int length = QAPI_LIST_LENGTH(cap);
+ init_multifd_array(length);
+ for (int i = 0; i < length; i++) {
+ const char *multifd_dst_uri = cap->value->destination_uri;
+ uint8_t multifd_channels = cap->value->multifd_channels;
+ qemu_start_incoming_migration(multifd_dst_uri, multifd_channels,
+ i, &local_err);
+ cap = cap->next;
+ }
+
+ if (local_err) {
+ yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+ error_propagate(errp, local_err);
+ return;
+ }
+ once = false;
+}
+
void qmp_migrate_recover(const char *uri, Error **errp)
{
MigrationIncomingState *mis = migration_incoming_get_current();
@@ -2246,7 +2349,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
* only re-setup the migration stream and poke existing migration
* to continue using that newly established channel.
*/
- qemu_start_incoming_migration(uri, errp);
+ qemu_start_incoming_migration(uri, 0, 0, errp);
}
void qmp_migrate_pause(Error **errp)
diff --git a/migration/migration.h b/migration/migration.h
index cdad8aceaa..4cb81f7cf0 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -394,6 +394,8 @@ bool migration_is_setup_or_active(int state);
bool migration_is_running(int state);
void migrate_init(MigrationState *s);
+void migrate_incoming(const char *uri, Error **errp);
+void multi_fd_migrate_incoming(const char *uri_str, Error **errp);
bool migration_is_blocked(Error **errp);
/* True if outgoing migration has entered postcopy phase */
bool migration_in_postcopy(void);
diff --git a/migration/socket.c b/migration/socket.c
index f199430bc2..dab872a0fe 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -227,17 +227,17 @@ socket_incoming_migration_end(void *opaque)
static void
socket_start_incoming_migration_internal(SocketAddress *saddr,
- Error **errp)
+ uint8_t multifd_count, Error **errp)
{
QIONetListener *listener = qio_net_listener_new();
MigrationIncomingState *mis = migration_incoming_get_current();
size_t i;
- int num = 1;
+ uint8_t num = 1;
qio_net_listener_set_name(listener, "migration-socket-listener");
if (migrate_use_multifd()) {
- num = migrate_multifd_channels();
+ num = multifd_count;
} else if (migrate_postcopy_preempt()) {
num = RAM_CHANNEL_MAX;
}
@@ -266,12 +266,13 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
}
}
-void socket_start_incoming_migration(const char *str, Error **errp)
+void socket_start_incoming_migration(const char *str,
+ uint8_t multifd_count, Error **errp)
{
Error *err = NULL;
SocketAddress *saddr = socket_parse(str, &err);
if (!err) {
- socket_start_incoming_migration_internal(saddr, &err);
+ socket_start_incoming_migration_internal(saddr, multifd_count, &err);
}
qapi_free_SocketAddress(saddr);
error_propagate(errp, err);
diff --git a/migration/socket.h b/migration/socket.h
index 0cbb7220ac..7c82278d33 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -33,7 +33,8 @@ void socket_send_channel_create(QIOTaskFunc f, void *data);
QIOChannel *socket_send_channel_create_sync(Error **errp);
int socket_send_channel_destroy(QIOChannel *send);
-void socket_start_incoming_migration(const char *str, Error **errp);
+void socket_start_incoming_migration(const char *str, uint8_t number,
+ Error **errp);
void socket_start_outgoing_migration(MigrationState *s, const char *dst_str,
Error **errp);
diff --git a/qapi/migration.json b/qapi/migration.json
index 456247af8f..3908c9096c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1526,14 +1526,36 @@
'*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
'*resume': 'bool' } }
+##
+# @MigrateIncomingUri:
+#
+# Information regarding which destination listening interface to be connected
+# and number of multifd channels over that interface.
+#
+# @destination-uri: the Uniform Resource Identifier of the destination VM
+#
+# @multifd-channels: number of channels used to migrate data in parallel for
+# for specific source-uri and destination-uri.
+# Default value in this case is 2 (Since 4.0)
+#
+##
+{ 'struct' : 'MigrateIncomingUri',
+ 'data' : { 'destination-uri' : 'str',
+ '*multifd-channels' : 'uint8'} }
+
##
# @migrate-incoming:
#
# Start an incoming migration, the qemu must have been started
-# with -incoming defer
+# with -incoming defer. qemu can also be started with optional
+# -multi-fd-incoming defer for opening multifd listening sockets
#
# @uri: The Uniform Resource Identifier identifying the source or
-# address to listen on
+# address to listen on.
+#
+# @multi-fd-uri-list: list of pair of source and destination VM Uniform
+# Resource Identifiers with number of multifd-channels
+# for each pair.
#
# Returns: nothing on success
#
@@ -1545,19 +1567,31 @@
# compatible with -incoming and the format of the uri is already exposed
# above libvirt.
#
-# 2. QEMU must be started with -incoming defer to allow migrate-incoming to
+# 2. multi-fd-uri-list will have list of destination uri as listening sockets
+# and multi-fd number of channels on each listening socket.
+#
+# 3. QEMU must be started with -incoming defer to allow migrate-incoming to
# be used.
#
-# 3. The uri format is the same as for -incoming
+# 4. multi-fd-uri-list format is not the same as for -multi-fd-incoming flag.
+# For -multi-fd-incoming flag, it is a comma separated list of listener
+# sockets with multifd channels.
+# Example: -multi-fd-incoming "tcp::6900:4,tcp:11.0.0.0:7789:5".
#
# Example:
#
# -> { "execute": "migrate-incoming",
-# "arguments": { "uri": "tcp::4446" } }
+# "arguments": {
+# "uri": "tcp::6789",
+# "multi-fd-uri-list" : [ { "destination-uri" : "tcp::6900",
+# "multifd-channels": 4},
+# { "destination-uri" : "tcp:11.0.0.0:7789",
+# "multifd-channels": 5} ] } }
# <- { "return": {} }
#
##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming',
+ 'data': {'uri': 'str', '*multi-fd-uri-list': ['MigrateIncomingUri'] } }
##
# @xen-save-devices-state:
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 9672ac6018..a256f12ad2 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -15,6 +15,7 @@
#include "qapi/error.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qapi-builtin-types.h"
CompatPolicy compat_policy;
@@ -157,7 +158,7 @@ int parse_qapi_name(const char *str, bool complete)
* Produce a strList from a delimiter separated list.
* A NULL or empty input string return NULL.
*/
-strList *strList_from_string(const char *in, char c)
+struct strList *strList_from_string(const char *in, char c)
{
strList *res = NULL;
strList **tail = &res;
diff --git a/qemu-options.hx b/qemu-options.hx
index 79e00916a1..5555f0d2fd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4479,6 +4479,24 @@ SRST
to issuing the migrate\_incoming to allow the migration to begin.
ERST
+DEF("multi-fd-incoming", HAS_ARG, QEMU_OPTION_multi_fd_incoming, \
+ "-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]\n" \
+ "-multi-fd-incoming defer\n" \
+ " wait for the URI to be specified via\n" \
+ " multi_fd_migrate_incoming\n",
+ QEMU_ARCH_ALL)
+SRST
+``-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]``
+ Prepare for multi-fd incoming migration, with multi-fd listening sockets
+ on that connection. Default number of multi-fd channels is 2.
+
+``-multi-fd-incoming defer``
+ Wait for the URI to be specified via multi_fd_migrate\_incoming. The
+ monitor can be used to change settings (such as migration parameters)
+ prior to issuing the multi_fd_migrate\_incoming to allow the migration
+ to begin.
+ERST
+
DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \
"-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL)
SRST
diff --git a/softmmu/vl.c b/softmmu/vl.c
index aabd82e09a..07c33ded20 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -45,7 +45,7 @@
#include "sysemu/seccomp.h"
#include "sysemu/tcg.h"
#include "sysemu/xen.h"
-
+#include "migration/migration.h"
#include "qemu/error-report.h"
#include "qemu/sockets.h"
#include "qemu/accel.h"
@@ -160,6 +160,7 @@ typedef struct DeviceOption {
static const char *cpu_option;
static const char *mem_path;
static const char *incoming;
+static const char *multi_fd_incoming;
static const char *loadvm;
static const char *accelerators;
static bool have_custom_ram_size;
@@ -2312,6 +2313,11 @@ static void qemu_validate_options(const QDict *machine_opts)
error_report("'preconfig' supports '-incoming defer' only");
exit(EXIT_FAILURE);
}
+ if (multi_fd_incoming && preconfig_requested &&
+ strcmp(multi_fd_incoming, "defer") != 0) {
+ error_report("'preconfig' supports '-multi-fd-incoming defer' only");
+ exit(EXIT_FAILURE);
+ }
#ifdef CONFIG_CURSES
if (is_daemonized() && dpy.type == DISPLAY_TYPE_CURSES) {
@@ -2600,7 +2606,7 @@ void qmp_x_exit_preconfig(Error **errp)
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
- qmp_migrate_incoming(incoming, &local_err);
+ migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
@@ -2609,6 +2615,20 @@ void qmp_x_exit_preconfig(Error **errp)
} else if (autostart) {
qmp_cont(NULL);
}
+
+ if (multi_fd_incoming) {
+ Error *local_err = NULL;
+ if (strcmp(multi_fd_incoming, "defer") != 0) {
+ multi_fd_migrate_incoming(multi_fd_incoming, &local_err);
+ if (local_err) {
+ error_reportf_err(local_err, "-multi-fd-incoming %s: ",
+ multi_fd_incoming);
+ exit(1);
+ }
+ }
+ } else if (autostart) {
+ qmp_cont(NULL);
+ }
}
void qemu_init(int argc, char **argv, char **envp)
@@ -3331,6 +3351,12 @@ void qemu_init(int argc, char **argv, char **envp)
}
incoming = optarg;
break;
+ case QEMU_OPTION_multi_fd_incoming:
+ if (!multi_fd_incoming) {
+ runstate_set(RUN_STATE_INMIGRATE);
+ }
+ multi_fd_incoming = optarg;
+ break;
case QEMU_OPTION_only_migratable:
only_migratable = 1;
break;
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 4/7] multifd: HMP changes for multifd source and destination side
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
` (2 preceding siblings ...)
2022-07-21 19:56 ` [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-21 19:56 ` [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair Het Gala
` (2 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) hmp_migrate and hmp_migrate_incoming functions are modified
according to the modified qmp monitor commands.
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
monitor/hmp-cmds.c | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8d25fee4be..ec12b950db 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -60,6 +60,9 @@
#include "migration/misc.h"
+/* Default number of multi-fd channels */
+#define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
+
#ifdef CONFIG_SPICE
#include <spice/enums.h>
#endif
@@ -1127,8 +1130,19 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
const char *uri = qdict_get_str(qdict, "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);
+ MigrateIncomingUriList *caps = NULL;
+ MigrateIncomingUri *value;
+
+ value = g_malloc0(sizeof(*value));
+ value->destination_uri = (char *)dst_uri;
+ value->multifd_channels = multifd_channels;
+ QAPI_LIST_PREPEND(caps, value);
- qmp_migrate_incoming(uri, &err);
+ qmp_migrate_incoming(uri, !!caps, caps, &err);
+ qapi_free_MigrateIncomingUriList(caps);
hmp_handle_error(mon, err);
}
@@ -1551,10 +1565,24 @@ 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, !!blk, blk, !!inc, inc,
- false, false, true, resume, &err);
+ qmp_migrate(uri, !!caps, caps, !!blk, blk, !!inc,
+ inc, false, false, true, resume, &err);
+ qapi_free_MigrateUriParameterList(caps);
if (hmp_handle_error(mon, err)) {
return;
}
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
` (3 preceding siblings ...)
2022-07-21 19:56 ` [PATCH v2 4/7] multifd: HMP changes for multifd source and " Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-26 10:44 ` Daniel P. Berrangé
2022-07-21 19:56 ` [PATCH v2 6/7] muitlfd: Correcting nit : whitespace error changes in qemu-sockets.c file Het Gala
2022-07-21 19:56 ` [PATCH v2 7/7] multifd: adding support for multifd connections dynamically Het Gala
6 siblings, 1 reply; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) Binding of the socket to source ip address and port on the non-default
interface has been implemented for multi-FD connection, which was not
necessary earlier because the binding was on the default interface itself.
ii) Created an end to end connection between all multi-FD source and
destination pairs.
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
include/io/channel-socket.h | 44 ++++++++++++++++
include/qemu/sockets.h | 6 ++-
io/channel-socket.c | 93 ++++++++++++++++++++++++++--------
migration/socket.c | 4 +-
tests/unit/test-util-sockets.c | 16 +++---
util/qemu-sockets.c | 90 +++++++++++++++++++++++---------
6 files changed, 196 insertions(+), 57 deletions(-)
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 513c428fe4..8168866b06 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -211,6 +211,50 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
GMainContext *context);
+/**
+ * qio_channel_socket_connect_all_sync:
+ * @ioc: the socket channel object
+ * @dst_addr: the destination address to connect to
+ * @src_addr: the source address to be connected
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Attempt to connect to the address @dst_addr with @src_addr.
+ * This method will run in the foreground so the caller will not
+ * regain execution control until the connection is established or
+ * an error occurs.
+ */
+int qio_channel_socket_connect_all_sync(QIOChannelSocket *ioc,
+ SocketAddress *dst_addr,
+ SocketAddress *src_addr,
+ Error **errp);
+
+/**
+ * qio_channel_socket_connect_all_async:
+ * @ioc: the socket channel object
+ * @dst_addr: the destination address to connect to
+ * @callback: the function to invoke on completion
+ * @opaque: user data to pass to @callback
+ * @destroy: the function to free @opaque
+ * @context: the context to run the async task. If %NULL, the default
+ * context will be used.
+ * @src_addr: the source address to be connected
+ *
+ * Attempt to connect to the address @dst_addr with the @src_addr.
+ * This method will run in the background so the caller will regain
+ * execution control immediately. The function @callback
+ * will be invoked on completion or failure. The @dst_addr
+ * parameter will be copied, so may be freed as soon
+ * as this function returns without waiting for completion.
+ */
+void qio_channel_socket_connect_all_async(QIOChannelSocket *ioc,
+ SocketAddress *dst_addr,
+ QIOTaskFunc callback,
+ gpointer opaque,
+ GDestroyNotify destroy,
+ GMainContext *context,
+ SocketAddress *src_addr);
+
+
/**
* qio_channel_socket_get_local_address:
* @ioc: the socket channel object
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 038faa157f..dc863c3df8 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -33,7 +33,8 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
Error **errp);
int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
int inet_connect(const char *str, Error **errp);
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
+int inet_connect_saddr(InetSocketAddress *dst_addr,
+ InetSocketAddress *src_addr, Error **errp);
NetworkAddressFamily inet_netfamily(int family);
@@ -41,7 +42,8 @@ int unix_listen(const char *path, Error **errp);
int unix_connect(const char *path, Error **errp);
SocketAddress *socket_parse(const char *str, Error **errp);
-int socket_connect(SocketAddress *addr, Error **errp);
+int socket_connect(SocketAddress *dst_addr, SocketAddress *src_addr,
+ Error **errp);
int socket_listen(SocketAddress *addr, int num, Error **errp);
void socket_listen_cleanup(int fd, Error **errp);
int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 74a936cc1f..142298469b 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -144,14 +144,15 @@ qio_channel_socket_new_fd(int fd,
}
-int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
- SocketAddress *addr,
- Error **errp)
+int qio_channel_socket_connect_all_sync(QIOChannelSocket *ioc,
+ SocketAddress *dst_addr,
+ SocketAddress *src_addr,
+ Error **errp)
{
int fd;
- trace_qio_channel_socket_connect_sync(ioc, addr);
- fd = socket_connect(addr, errp);
+ trace_qio_channel_socket_connect_sync(ioc, dst_addr);
+ fd = socket_connect(dst_addr, src_addr, errp);
if (fd < 0) {
trace_qio_channel_socket_connect_fail(ioc);
return -1;
@@ -177,19 +178,78 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
}
-static void qio_channel_socket_connect_worker(QIOTask *task,
- gpointer opaque)
+struct ConnectData {
+ SocketAddress *dst_addr;
+ SocketAddress *src_addr;
+};
+
+
+static void qio_channel_socket_all_worker_free(gpointer opaque)
+{
+ struct ConnectData *data = opaque;
+ if (!data) {
+ return;
+ }
+ qapi_free_SocketAddress(data->dst_addr);
+ qapi_free_SocketAddress(data->src_addr);
+ g_free(data);
+}
+
+static void qio_channel_socket_connect_all_worker(QIOTask *task,
+ gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
- SocketAddress *addr = opaque;
+ struct ConnectData *data = opaque;
Error *err = NULL;
- qio_channel_socket_connect_sync(ioc, addr, &err);
+ qio_channel_socket_connect_all_sync(ioc, data->dst_addr,
+ data->src_addr, &err);
qio_task_set_error(task, err);
}
+void qio_channel_socket_connect_all_async(QIOChannelSocket *ioc,
+ SocketAddress *dst_addr,
+ QIOTaskFunc callback,
+ gpointer opaque,
+ GDestroyNotify destroy,
+ GMainContext *context,
+ SocketAddress *src_addr)
+{
+ QIOTask *task = qio_task_new(
+ OBJECT(ioc), callback, opaque, destroy);
+ struct ConnectData *data = g_new0(struct ConnectData, 1);
+
+ data->dst_addr = QAPI_CLONE(SocketAddress, dst_addr);
+ if (src_addr) {
+ data->src_addr = QAPI_CLONE(SocketAddress, src_addr);
+ } else {
+ data->src_addr = NULL;
+ }
+ /*
+ * socket_connect() does a non-blocking connect(), but it
+ * still blocks in DNS lookups, so we must use a thread
+ */
+ trace_qio_channel_socket_connect_async(ioc, dst_addr);
+ qio_task_run_in_thread(task,
+ qio_channel_socket_connect_all_worker,
+ data,
+ qio_channel_socket_all_worker_free,
+ context);
+}
+
+
+int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
+ SocketAddress *addr,
+ Error **errp)
+{
+ qio_channel_socket_connect_all_sync(ioc, addr, NULL, errp);
+
+ return 0;
+}
+
+
void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
SocketAddress *addr,
QIOTaskFunc callback,
@@ -197,20 +257,9 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
GDestroyNotify destroy,
GMainContext *context)
{
- QIOTask *task = qio_task_new(
- OBJECT(ioc), callback, opaque, destroy);
- SocketAddress *addrCopy;
-
- addrCopy = QAPI_CLONE(SocketAddress, addr);
+ qio_channel_socket_connect_all_async(ioc, addr, callback, opaque,
+ destroy, context, NULL);
- /* socket_connect() does a non-blocking connect(), but it
- * still blocks in DNS lookups, so we must use a thread */
- trace_qio_channel_socket_connect_async(ioc, addr);
- qio_task_run_in_thread(task,
- qio_channel_socket_connect_worker,
- addrCopy,
- (GDestroyNotify)qapi_free_SocketAddress,
- context);
}
diff --git a/migration/socket.c b/migration/socket.c
index dab872a0fe..69fda774ba 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -57,8 +57,8 @@ int outgoing_param_total_multifds(void)
void socket_send_channel_create(QIOTaskFunc f, void *data)
{
QIOChannelSocket *sioc = qio_channel_socket_new();
- qio_channel_socket_connect_async(sioc, outgoing_args.saddr,
- f, data, NULL, NULL);
+ qio_channel_socket_connect_all_async(sioc, outgoing_args.saddr,
+ f, data, NULL, NULL, NULL);
}
QIOChannel *socket_send_channel_create_sync(Error **errp)
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index 63909ccb2b..aa26630045 100644
--- a/tests/unit/test-util-sockets.c
+++ b/tests/unit/test-util-sockets.c
@@ -89,7 +89,7 @@ static void test_socket_fd_pass_name_good(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup(mon_fdname);
- fd = socket_connect(&addr, &error_abort);
+ fd = socket_connect(&addr, NULL, &error_abort);
g_assert_cmpint(fd, !=, -1);
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
@@ -121,7 +121,7 @@ static void test_socket_fd_pass_name_bad(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup(mon_fdname);
- fd = socket_connect(&addr, &err);
+ fd = socket_connect(&addr, NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -148,7 +148,7 @@ static void test_socket_fd_pass_name_nomon(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup("myfd");
- fd = socket_connect(&addr, &err);
+ fd = socket_connect(&addr, NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -172,7 +172,7 @@ static void test_socket_fd_pass_num_good(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup_printf("%d", sfd);
- fd = socket_connect(&addr, &error_abort);
+ fd = socket_connect(&addr, NULL, &error_abort);
g_assert_cmpint(fd, ==, sfd);
fd = socket_listen(&addr, 1, &error_abort);
@@ -194,7 +194,7 @@ static void test_socket_fd_pass_num_bad(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup_printf("%d", sfd);
- fd = socket_connect(&addr, &err);
+ fd = socket_connect(&addr, NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -217,7 +217,7 @@ static void test_socket_fd_pass_num_nocli(void)
addr.type = SOCKET_ADDRESS_TYPE_FD;
addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO);
- fd = socket_connect(&addr, &err);
+ fd = socket_connect(&addr, NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -246,10 +246,10 @@ static gpointer unix_client_thread_func(gpointer user_data)
for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
if (row->expect_connect[i]) {
- fd = socket_connect(row->client[i], &error_abort);
+ fd = socket_connect(row->client[i], NULL, &error_abort);
g_assert_cmpint(fd, >=, 0);
} else {
- fd = socket_connect(row->client[i], &err);
+ fd = socket_connect(row->client[i], NULL, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
}
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 13b5b197f9..491e2f2bc9 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -358,8 +358,10 @@ listen_ok:
((rc) == -EINPROGRESS)
#endif
-static int inet_connect_addr(const InetSocketAddress *saddr,
- struct addrinfo *addr, Error **errp)
+static int inet_connect_addr(const InetSocketAddress *dst_addr,
+ const InetSocketAddress *src_addr,
+ struct addrinfo *addr, struct addrinfo *saddr,
+ Error **errp)
{
int sock, rc;
@@ -371,8 +373,28 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
}
socket_set_fast_reuse(sock);
+ /* to bind the socket if src_addr is available */
+
+ if (src_addr) {
+ struct sockaddr_in servaddr;
+
+ /* bind to a specific interface in the internet domain */
+ /* to make sure the sin_zero filed is cleared */
+ memset(&servaddr, 0, sizeof(servaddr));
+
+ servaddr.sin_family = AF_INET;
+ servaddr.sin_addr.s_addr = inet_addr(src_addr->host);
+ servaddr.sin_port = 0;
+
+ if (bind(sock, (struct sockaddr *)&servaddr, sizeof(servaddr)) < 0) {
+ error_setg_errno(errp, errno, "Failed to bind socket");
+ return -1;
+ }
+ }
+
/* connect to peer */
do {
+
rc = 0;
if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
rc = -errno;
@@ -380,8 +402,14 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
} while (rc == -EINTR);
if (rc < 0) {
- error_setg_errno(errp, errno, "Failed to connect to '%s:%s'",
- saddr->host, saddr->port);
+ if (src_addr) {
+ error_setg_errno(errp, errno, "Failed to connect '%s:%s' to "
+ "'%s:%s'", dst_addr->host, dst_addr->port,
+ src_addr->host, src_addr->port);
+ } else {
+ error_setg_errno(errp, errno, "Failed to connect '%s:%s'",
+ dst_addr->host, dst_addr->port);
+ }
closesocket(sock);
return -1;
}
@@ -446,41 +474,55 @@ static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
*
* Returns: -1 on error, file descriptor on success.
*/
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
+int inet_connect_saddr(InetSocketAddress *dst_addr,
+ InetSocketAddress *src_addr, Error **errp)
{
Error *local_err = NULL;
- struct addrinfo *res, *e;
+ struct addrinfo *res_d, *res_s, *e, *x;
int sock = -1;
- res = inet_parse_connect_saddr(saddr, errp);
- if (!res) {
+ res_d = inet_parse_connect_saddr(dst_addr, errp);
+ if (!res_d) {
return -1;
}
- for (e = res; e != NULL; e = e->ai_next) {
+ if (src_addr) {
+ res_s = inet_parse_connect_saddr(src_addr, errp);
+ if (!res_s) {
+ return -1;
+ }
+ }
+
+ for (e = res_d; e != NULL; e = e->ai_next) {
error_free(local_err);
local_err = NULL;
#ifdef HAVE_IPPROTO_MPTCP
- if (saddr->has_mptcp && saddr->mptcp) {
+ if (dst_addr->has_mptcp && dst_addr->mptcp) {
e->ai_protocol = IPPROTO_MPTCP;
}
#endif
+ for (x = res_s; x != NULL; x = x->ai_next) {
+ if (!src_addr || e->ai_family == x->ai_family) {
- sock = inet_connect_addr(saddr, e, &local_err);
- if (sock >= 0) {
- break;
+ sock = inet_connect_addr(dst_addr, src_addr,
+ e, x, &local_err);
+ if (sock >= 0) {
+ break;
+ }
+ }
}
}
- freeaddrinfo(res);
+ freeaddrinfo(res_d);
+ freeaddrinfo(res_s);
if (sock < 0) {
error_propagate(errp, local_err);
return sock;
}
- if (saddr->keep_alive) {
+ if (dst_addr->keep_alive) {
int val = 1;
int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
&val, sizeof(val));
@@ -506,7 +548,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
Error *err = NULL;
/* lookup peer addr */
- memset(&ai,0, sizeof(ai));
+ memset(&ai,0,sizeof(ai));
ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
ai.ai_family = inet_ai_family_from_address(sraddr, &err);
ai.ai_socktype = SOCK_DGRAM;
@@ -727,7 +769,7 @@ int inet_connect(const char *str, Error **errp)
InetSocketAddress *addr = g_new(InetSocketAddress, 1);
if (!inet_parse(addr, str, errp)) {
- sock = inet_connect_saddr(addr, errp);
+ sock = inet_connect_saddr(addr, NULL, errp);
}
qapi_free_InetSocketAddress(addr);
return sock;
@@ -1182,25 +1224,27 @@ int socket_address_parse_named_fd(SocketAddress *addr, Error **errp)
return 0;
}
-int socket_connect(SocketAddress *addr, Error **errp)
+int socket_connect(SocketAddress *dst_addr,
+ SocketAddress *src_addr, Error **errp)
{
int fd;
- switch (addr->type) {
+ switch (dst_addr->type) {
case SOCKET_ADDRESS_TYPE_INET:
- fd = inet_connect_saddr(&addr->u.inet, errp);
+ fd = inet_connect_saddr(&dst_addr->u.inet, src_addr ?
+ &src_addr->u.inet : NULL, errp);
break;
case SOCKET_ADDRESS_TYPE_UNIX:
- fd = unix_connect_saddr(&addr->u.q_unix, errp);
+ fd = unix_connect_saddr(&dst_addr->u.q_unix, errp);
break;
case SOCKET_ADDRESS_TYPE_FD:
- fd = socket_get_fd(addr->u.fd.str, errp);
+ fd = socket_get_fd(dst_addr->u.fd.str, errp);
break;
case SOCKET_ADDRESS_TYPE_VSOCK:
- fd = vsock_connect_saddr(&addr->u.vsock, errp);
+ fd = vsock_connect_saddr(&dst_addr->u.vsock, errp);
break;
default:
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 6/7] muitlfd: Correcting nit : whitespace error changes in qemu-sockets.c file
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
` (4 preceding siblings ...)
2022-07-21 19:56 ` [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair Het Gala
@ 2022-07-21 19:56 ` Het Gala
2022-07-21 19:56 ` [PATCH v2 7/7] multifd: adding support for multifd connections dynamically Het Gala
6 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
util/qemu-sockets.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 491e2f2bc9..724c081e6c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -52,10 +52,10 @@ static int inet_getport(struct addrinfo *e)
switch (e->ai_family) {
case PF_INET6:
- i6 = (void*)e->ai_addr;
+ i6 = (void *)e->ai_addr;
return ntohs(i6->sin6_port);
case PF_INET:
- i4 = (void*)e->ai_addr;
+ i4 = (void *)e->ai_addr;
return ntohs(i4->sin_port);
default:
return 0;
@@ -69,11 +69,11 @@ static void inet_setport(struct addrinfo *e, int port)
switch (e->ai_family) {
case PF_INET6:
- i6 = (void*)e->ai_addr;
+ i6 = (void *)e->ai_addr;
i6->sin6_port = htons(port);
break;
case PF_INET:
- i4 = (void*)e->ai_addr;
+ i4 = (void *)e->ai_addr;
i4->sin_port = htons(port);
break;
}
@@ -210,9 +210,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
int num,
Error **errp)
{
- struct addrinfo ai,*res,*e;
+ struct addrinfo ai, *res, *e;
char port[33];
- char uaddr[INET6_ADDRSTRLEN+1];
+ char uaddr[INET6_ADDRSTRLEN + 1];
char uport[33];
int rc, port_min, port_max, p;
int slisten = -1;
@@ -226,7 +226,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
return -1;
}
- memset(&ai,0, sizeof(ai));
+ memset(&ai, 0, sizeof(ai));
ai.ai_flags = AI_PASSIVE;
if (saddr->has_numeric && saddr->numeric) {
ai.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV;
@@ -282,8 +282,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
e->ai_protocol = IPPROTO_MPTCP;
}
#endif
- getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
- uaddr,INET6_ADDRSTRLEN,uport,32,
+ getnameinfo((struct sockaddr *)e->ai_addr, e->ai_addrlen,
+ uaddr, INET6_ADDRSTRLEN, uport, 32,
NI_NUMERICHOST | NI_NUMERICSERV);
port_min = inet_getport(e);
@@ -548,7 +548,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
Error *err = NULL;
/* lookup peer addr */
- memset(&ai,0,sizeof(ai));
+ memset(&ai, 0, sizeof(ai));
ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
ai.ai_family = inet_ai_family_from_address(sraddr, &err);
ai.ai_socktype = SOCK_DGRAM;
@@ -575,7 +575,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
}
/* lookup local addr */
- memset(&ai,0, sizeof(ai));
+ memset(&ai, 0, sizeof(ai));
ai.ai_flags = AI_PASSIVE;
ai.ai_family = peer->ai_family;
ai.ai_socktype = SOCK_DGRAM;
@@ -616,7 +616,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
}
/* connect to peer */
- if (connect(sock,peer->ai_addr,peer->ai_addrlen) < 0) {
+ if (connect(sock, peer->ai_addr, peer->ai_addrlen) < 0) {
error_setg_errno(errp, errno, "Failed to connect to '%s:%s'",
addr, port);
goto err;
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 7/7] multifd: adding support for multifd connections dynamically
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
` (5 preceding siblings ...)
2022-07-21 19:56 ` [PATCH v2 6/7] muitlfd: Correcting nit : whitespace error changes in qemu-sockets.c file Het Gala
@ 2022-07-21 19:56 ` Het Gala
6 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-21 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: quintela, dgilbert, pbonzini, berrange, armbru, eblake,
prerna.saxena, Het Gala, Manish Mishra
i) Dynamically decide appropriate source and destination ip pairs for the
corresponding multifd channel to be connected.
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/multifd.c | 6 +++---
migration/socket.c | 37 ++++++++++++++++++++++---------------
migration/socket.h | 3 ++-
3 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 586ddc9d65..2d9d50239a 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -903,7 +903,7 @@ int multifd_save_setup(Error **errp)
int thread_count;
uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
uint8_t i;
-
+ int idx;
if (!migrate_use_multifd()) {
return 0;
}
@@ -945,8 +945,8 @@ int multifd_save_setup(Error **errp)
} else {
p->write_flags = 0;
}
-
- socket_send_channel_create(multifd_new_send_channel_async, p);
+ idx = multifd_index(i);
+ socket_send_channel_create(multifd_new_send_channel_async, p, idx);
}
for (i = 0; i < thread_count; i++) {
diff --git a/migration/socket.c b/migration/socket.c
index 69fda774ba..b81b819584 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -28,10 +28,6 @@
#include "trace.h"
#include "postcopy-ram.h"
-struct SocketOutgoingArgs {
- SocketAddress *saddr;
-} outgoing_args;
-
struct SocketArgs {
struct SrcDestAddr address;
uint8_t multifd_channels;
@@ -54,11 +50,30 @@ int outgoing_param_total_multifds(void)
return total_multifd_channels;
}
-void socket_send_channel_create(QIOTaskFunc f, void *data)
+
+int multifd_index(int i)
+{
+ int length = outgoing_migrate_params.socket_args_arr_len;
+ int j = 0;
+ int runn_sum = 0;
+ while (j < length) {
+ runn_sum += outgoing_migrate_params.socket_args[j].multifd_channels;
+ if (i >= runn_sum) {
+ j++;
+ } else {
+ break;
+ }
+ }
+ return j;
+}
+
+ void socket_send_channel_create(QIOTaskFunc f, void *data, int idx)
{
QIOChannelSocket *sioc = qio_channel_socket_new();
- qio_channel_socket_connect_all_async(sioc, outgoing_args.saddr,
- f, data, NULL, NULL, NULL);
+ qio_channel_socket_connect_all_async(sioc,
+ outgoing_migrate_params.socket_args[idx].address.dst_addr,
+ f, data, NULL, NULL,
+ outgoing_migrate_params.socket_args[idx].address.src_addr);
}
QIOChannel *socket_send_channel_create_sync(Error **errp)
@@ -83,10 +98,6 @@ int socket_send_channel_destroy(QIOChannel *send)
{
/* Remove channel */
object_unref(OBJECT(send));
- if (outgoing_args.saddr) {
- qapi_free_SocketAddress(outgoing_args.saddr);
- outgoing_args.saddr = NULL;
- }
g_free(outgoing_migrate_params.socket_args);
outgoing_migrate_params.socket_args = NULL;
outgoing_migrate_params.socket_args_arr_len = 0;
@@ -142,10 +153,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
data->s = s;
- /* in case previous migration leaked it */
- qapi_free_SocketAddress(outgoing_args.saddr);
- outgoing_args.saddr = saddr;
-
if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
data->hostname = g_strdup(saddr->u.inet.host);
}
diff --git a/migration/socket.h b/migration/socket.h
index 7c82278d33..80d03fc7d6 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -29,7 +29,8 @@ typedef struct SrcDestAddr {
int outgoing_param_total_multifds(void);
-void socket_send_channel_create(QIOTaskFunc f, void *data);
+int multifd_index(int i);
+void socket_send_channel_create(QIOTaskFunc f, void *data, int idx);
QIOChannel *socket_send_channel_create_sync(Error **errp);
int socket_send_channel_destroy(QIOChannel *send);
--
2.22.3
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair
2022-07-21 19:56 ` [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair Het Gala
@ 2022-07-26 10:44 ` Daniel P. Berrangé
2022-07-28 15:15 ` Het Gala
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-26 10:44 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
In $SUBJECT s/multifd:/io:/ as this is not migration related.
On Thu, Jul 21, 2022 at 07:56:18PM +0000, Het Gala wrote:
> i) Binding of the socket to source ip address and port on the non-default
> interface has been implemented for multi-FD connection, which was not
> necessary earlier because the binding was on the default interface itself.
>
> ii) Created an end to end connection between all multi-FD source and
> destination pairs.
>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> include/io/channel-socket.h | 44 ++++++++++++++++
> include/qemu/sockets.h | 6 ++-
> io/channel-socket.c | 93 ++++++++++++++++++++++++++--------
> migration/socket.c | 4 +-
> tests/unit/test-util-sockets.c | 16 +++---
> util/qemu-sockets.c | 90 +++++++++++++++++++++++---------
> 6 files changed, 196 insertions(+), 57 deletions(-)
>
> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index 513c428fe4..8168866b06 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -211,6 +211,50 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
> GMainContext *context);
>
>
> +/**
> + * qio_channel_socket_connect_all_sync:
This needs to be called qio_channel_socket_connect_full_sync to
match the naming conventions in use in IO code.
> + * @ioc: the socket channel object
> + * @dst_addr: the destination address to connect to
> + * @src_addr: the source address to be connected
'the optional source address to bind to'
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Attempt to connect to the address @dst_addr with @src_addr.
* Attempt to connect to the address @dst_addr. If @src_addr
* is non-NULL, it will be bound to in order to control outbound
* interface routing.
> + * This method will run in the foreground so the caller will not
> + * regain execution control until the connection is established or
> + * an error occurs.
> + */
> +int qio_channel_socket_connect_all_sync(QIOChannelSocket *ioc,
> + SocketAddress *dst_addr,
> + SocketAddress *src_addr,
> + Error **errp);
Vertical mis-alignment of parameters
> +
> +/**
> + * qio_channel_socket_connect_all_async:
Needs to be qio_channel_socket_connect_full_async
> + * @ioc: the socket channel object
> + * @dst_addr: the destination address to connect to
@src_addr needs to be placed here...
> + * @callback: the function to invoke on completion
> + * @opaque: user data to pass to @callback
> + * @destroy: the function to free @opaque
> + * @context: the context to run the async task. If %NULL, the default
> + * context will be used.
> + * @src_addr: the source address to be connected
...not here
and same note about the docs comment
> + *
> + * Attempt to connect to the address @dst_addr with the @src_addr.
Same note about the docs comment
> + * This method will run in the background so the caller will regain
> + * execution control immediately. The function @callback
> + * will be invoked on completion or failure. The @dst_addr
> + * parameter will be copied, so may be freed as soon
> + * as this function returns without waiting for completion.
> + */
> +void qio_channel_socket_connect_all_async(QIOChannelSocket *ioc,
> + SocketAddress *dst_addr,
> + QIOTaskFunc callback,
> + gpointer opaque,
> + GDestroyNotify destroy,
> + GMainContext *context,
> + SocketAddress *src_addr);
> +
> +
> /**
> * qio_channel_socket_get_local_address:
> * @ioc: the socket channel object
> diff --git a/migration/socket.c b/migration/socket.c
> index dab872a0fe..69fda774ba 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -57,8 +57,8 @@ int outgoing_param_total_multifds(void)
> void socket_send_channel_create(QIOTaskFunc f, void *data)
> {
> QIOChannelSocket *sioc = qio_channel_socket_new();
> - qio_channel_socket_connect_async(sioc, outgoing_args.saddr,
> - f, data, NULL, NULL);
> + qio_channel_socket_connect_all_async(sioc, outgoing_args.saddr,
> + f, data, NULL, NULL, NULL);
> }
Don't change this call at all until the next patch which actually
needs to pass a non-NULL parameter for src.
> QIOChannel *socket_send_channel_create_sync(Error **errp)
> diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> index 63909ccb2b..aa26630045 100644
> --- a/tests/unit/test-util-sockets.c
> +++ b/tests/unit/test-util-sockets.c
> @@ -89,7 +89,7 @@ static void test_socket_fd_pass_name_good(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup(mon_fdname);
>
> - fd = socket_connect(&addr, &error_abort);
> + fd = socket_connect(&addr, NULL, &error_abort);
> g_assert_cmpint(fd, !=, -1);
> g_assert_cmpint(fd, !=, mon_fd);
> close(fd);
> @@ -121,7 +121,7 @@ static void test_socket_fd_pass_name_bad(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup(mon_fdname);
>
> - fd = socket_connect(&addr, &err);
> + fd = socket_connect(&addr, NULL, &err);
> g_assert_cmpint(fd, ==, -1);
> error_free_or_abort(&err);
>
> @@ -148,7 +148,7 @@ static void test_socket_fd_pass_name_nomon(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup("myfd");
>
> - fd = socket_connect(&addr, &err);
> + fd = socket_connect(&addr, NULL, &err);
> g_assert_cmpint(fd, ==, -1);
> error_free_or_abort(&err);
>
> @@ -172,7 +172,7 @@ static void test_socket_fd_pass_num_good(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup_printf("%d", sfd);
>
> - fd = socket_connect(&addr, &error_abort);
> + fd = socket_connect(&addr, NULL, &error_abort);
> g_assert_cmpint(fd, ==, sfd);
>
> fd = socket_listen(&addr, 1, &error_abort);
> @@ -194,7 +194,7 @@ static void test_socket_fd_pass_num_bad(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup_printf("%d", sfd);
>
> - fd = socket_connect(&addr, &err);
> + fd = socket_connect(&addr, NULL, &err);
> g_assert_cmpint(fd, ==, -1);
> error_free_or_abort(&err);
>
> @@ -217,7 +217,7 @@ static void test_socket_fd_pass_num_nocli(void)
> addr.type = SOCKET_ADDRESS_TYPE_FD;
> addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO);
>
> - fd = socket_connect(&addr, &err);
> + fd = socket_connect(&addr, NULL, &err);
> g_assert_cmpint(fd, ==, -1);
> error_free_or_abort(&err);
>
> @@ -246,10 +246,10 @@ static gpointer unix_client_thread_func(gpointer user_data)
>
> for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
> if (row->expect_connect[i]) {
> - fd = socket_connect(row->client[i], &error_abort);
> + fd = socket_connect(row->client[i], NULL, &error_abort);
> g_assert_cmpint(fd, >=, 0);
> } else {
> - fd = socket_connect(row->client[i], &err);
> + fd = socket_connect(row->client[i], NULL, &err);
> g_assert_cmpint(fd, ==, -1);
> error_free_or_abort(&err);
> }
I'd expect something added to the test suite to exercise the new
codepath. Obviously we'd be limted to dealing with 127.0.0.1,
but it can at least run the code paths.
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 13b5b197f9..491e2f2bc9 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -358,8 +358,10 @@ listen_ok:
> ((rc) == -EINPROGRESS)
> #endif
>
> -static int inet_connect_addr(const InetSocketAddress *saddr,
> - struct addrinfo *addr, Error **errp)
> +static int inet_connect_addr(const InetSocketAddress *dst_addr,
> + const InetSocketAddress *src_addr,
> + struct addrinfo *addr, struct addrinfo *saddr,
> + Error **errp)
> {
> int sock, rc;
>
> @@ -371,8 +373,28 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
> }
> socket_set_fast_reuse(sock);
>
> + /* to bind the socket if src_addr is available */
> +
> + if (src_addr) {
> + struct sockaddr_in servaddr;
> +
> + /* bind to a specific interface in the internet domain */
> + /* to make sure the sin_zero filed is cleared */
> + memset(&servaddr, 0, sizeof(servaddr));
> +
> + servaddr.sin_family = AF_INET;
> + servaddr.sin_addr.s_addr = inet_addr(src_addr->host);
My feedback from the previous posting has been ignored. This code is
broken for IPv6. Never call the IPv4-only APIs, getaddrinfo is the
only way to get a 'struct sockaddr *' in a protocol portable manner.
> + servaddr.sin_port = 0;
> +
> + if (bind(sock, (struct sockaddr *)&servaddr, sizeof(servaddr)) < 0) {
> + error_setg_errno(errp, errno, "Failed to bind socket");
> + return -1;
> + }
> + }
> +
> /* connect to peer */
> do {
> +
Arbitrary whitespace change should be removed
> rc = 0;
> if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
> rc = -errno;
> @@ -380,8 +402,14 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
> @@ -446,41 +474,55 @@ static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
> *
> * Returns: -1 on error, file descriptor on success.
> */
> -int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
> +int inet_connect_saddr(InetSocketAddress *dst_addr,
> + InetSocketAddress *src_addr, Error **errp)
> {
> Error *local_err = NULL;
> - struct addrinfo *res, *e;
> + struct addrinfo *res_d, *res_s, *e, *x;
> int sock = -1;
>
> - res = inet_parse_connect_saddr(saddr, errp);
> - if (!res) {
> + res_d = inet_parse_connect_saddr(dst_addr, errp);
> + if (!res_d) {
> return -1;
> }
>
> - for (e = res; e != NULL; e = e->ai_next) {
> + if (src_addr) {
> + res_s = inet_parse_connect_saddr(src_addr, errp);
> + if (!res_s) {
> + return -1;
> + }
> + }
> +
> + for (e = res_d; e != NULL; e = e->ai_next) {
> error_free(local_err);
> local_err = NULL;
>
> #ifdef HAVE_IPPROTO_MPTCP
> - if (saddr->has_mptcp && saddr->mptcp) {
> + if (dst_addr->has_mptcp && dst_addr->mptcp) {
> e->ai_protocol = IPPROTO_MPTCP;
> }
> #endif
> + for (x = res_s; x != NULL; x = x->ai_next) {
> + if (!src_addr || e->ai_family == x->ai_family) {
>
> - sock = inet_connect_addr(saddr, e, &local_err);
> - if (sock >= 0) {
> - break;
> + sock = inet_connect_addr(dst_addr, src_addr,
> + e, x, &local_err);
> + if (sock >= 0) {
> + break;
> + }
> + }
> }
> }
If the ai_family for the src is different from ai_family for
the dst, this loop will never call inet_connect_addr at all,
and leave local_err unset, and so the error_propagate call
below will have no error message to propagate.
>
> - freeaddrinfo(res);
> + freeaddrinfo(res_d);
> + freeaddrinfo(res_s);
>
> if (sock < 0) {
> error_propagate(errp, local_err);
> return sock;
> }
>
> - if (saddr->keep_alive) {
> + if (dst_addr->keep_alive) {
> int val = 1;
> int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> &val, sizeof(val));
> @@ -506,7 +548,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
> Error *err = NULL;
>
> /* lookup peer addr */
> - memset(&ai,0, sizeof(ai));
> + memset(&ai,0,sizeof(ai));
Unrelated whitespace change.
> ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
> ai.ai_family = inet_ai_family_from_address(sraddr, &err);
> ai.ai_socktype = SOCK_DGRAM;
> @@ -727,7 +769,7 @@ int inet_connect(const char *str, Error **errp)
> InetSocketAddress *addr = g_new(InetSocketAddress, 1);
>
> if (!inet_parse(addr, str, errp)) {
> - sock = inet_connect_saddr(addr, errp);
> + sock = inet_connect_saddr(addr, NULL, errp);
> }
> qapi_free_InetSocketAddress(addr);
> return sock;
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-07-21 19:56 ` [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair Het Gala
@ 2022-07-26 11:13 ` Daniel P. Berrangé
2022-07-28 15:02 ` Het Gala
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-26 11:13 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
> each element in the list consisting of multifd connection parameters: source
> uri, destination uri and of the number of multifd channels between each pair.
>
> ii) Information of all multifd connection parameters' list and length of the
> list is stored in 'OutgoingMigrateParams' struct.
>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
> migration/socket.h | 19 +++++++++++++-
> monitor/hmp-cmds.c | 1 +
> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
> 5 files changed, 160 insertions(+), 19 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 81185d4311..456247af8f 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,37 @@
> ##
> { '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: uri of the source VM. Default port number is 0.
> +#
> +# @destination-uri: uri 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 7.1)
> +#
> +##
> +{ '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)
> #
> @@ -1474,20 +1499,32 @@
> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
> +# 3. All boolean arguments default to false
> +#
> +# 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' } }
Considering the existing migrate API from a QAPI design POV, I
think there are several significant flaws with it
The use of URIs is the big red flag. It is basically a data encoding
scheme within a data encoding scheme. QEMU code should be able to
directly work with the results from QAPI, without having todo a
second level of parsing.
URIs made sense in the context of HMP or the QemuOpts CLI, but do not
make sense in QMP. We made a mistake in this respect when we first
introduced QMP and implemented 'migrate'.
If we going to extend the migrate API I think we should stop using URIs
for the new fields, and instead define a QAPI discriminated union for
the different data transport backends we offer.
{ 'enum': 'MigrateTransport',
'data': ['socket', 'exec'] }
{ 'union': 'MigrateAddress',
'base': { 'transport': 'MigrateTransport'},
'discriminator': 'transport',
'data': {
'socket': 'SocketAddress',
'exec': ['str'],
}
NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
and 'fd' already. I'm fuzzy on best way to represent RDMA.
IIUC, the desire of migration maintainers is that we can ultimately
have multifd as the preferred, or even only, mechanism. Aside from
the main outbound migration control channel, and the multifd
data channels, IIUC we have a potential desire to have more channels
for post-copy async requests.
This all suggests to me a more general representation along the
lines of:
{ 'enum': 'MigrateChannelType',
'data': ['control', 'data', 'async'] }
{ 'struct': 'MigrateChannel',
'data': {
'type': 'MigrateChannelType',
'src-addr': 'MigrateAddress',
'dst-addr': 'MigrateAddress',
'count': 'int',
} }
{ 'comand': 'migrate',
'data': {
'*uri': 'str'
'*channels': ['MigrateChannel']
}
}
With 'uri' and 'channels' being mutually exclusive here.
This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
API wrt multifd - essentally the same data is now being set in two
different places. IMHO, we should declare the 'multifd' capability
and the 'multifd-chanels' parameter deprecated, since the information
they provide is totally redundant, if you're giving an explicit list
of channels to 'migrate'.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side
2022-07-21 19:56 ` [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side Het Gala
@ 2022-07-26 11:20 ` Daniel P. Berrangé
2022-07-28 15:05 ` Het Gala
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-26 11:20 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On Thu, Jul 21, 2022 at 07:56:16PM +0000, Het Gala wrote:
> i) Modified the format of qemu monitor command: 'migrate-incoming' by adding
> a list, each element in the list to open socket listeners with a given
> number of multifd channels.
>
> ii) Qemu starts with -incoming flag defer and -multi-fd-incoming defer to
> allow the modified 'migrate-incoming' command to be used.
>
> iii) Format for -multi-fd-incoming flag as a comma separated string has been
> added with each substring containing listener socket address and number
> of sockets to open.
>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 79e00916a1..5555f0d2fd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4479,6 +4479,24 @@ SRST
> to issuing the migrate\_incoming to allow the migration to begin.
> ERST
>
> +DEF("multi-fd-incoming", HAS_ARG, QEMU_OPTION_multi_fd_incoming, \
> + "-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]\n" \
> + "-multi-fd-incoming defer\n" \
> + " wait for the URI to be specified via\n" \
> + " multi_fd_migrate_incoming\n",
> + QEMU_ARCH_ALL)
> +SRST
> +``-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]``
> + Prepare for multi-fd incoming migration, with multi-fd listening sockets
> + on that connection. Default number of multi-fd channels is 2.
> +
> +``-multi-fd-incoming defer``
> + Wait for the URI to be specified via multi_fd_migrate\_incoming. The
> + monitor can be used to change settings (such as migration parameters)
> + prior to issuing the multi_fd_migrate\_incoming to allow the migration
> + to begin.
> +ERST
We should not be adding any new -multi-fd-incoming CLI parameter at all.
The CLI is so unsuitable for any complex configuration param and this
is a prime example.
If anything we should fully deprecate anything that is not '-incoming defer'
such that we become 100% QMP/QAPI based for incoming migration config.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-07-26 11:13 ` Daniel P. Berrangé
@ 2022-07-28 15:02 ` Het Gala
2022-08-02 7:53 ` Markus Armbruster
2022-08-08 9:29 ` Daniel P. Berrangé
0 siblings, 2 replies; 22+ messages in thread
From: Het Gala @ 2022-07-28 15:02 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
> On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
>> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
>> each element in the list consisting of multifd connection parameters: source
>> uri, destination uri and of the number of multifd channels between each pair.
>>
>> ii) Information of all multifd connection parameters' list and length of the
>> list is stored in 'OutgoingMigrateParams' struct.
>>
>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
>> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>> migration/socket.h | 19 +++++++++++++-
>> monitor/hmp-cmds.c | 1 +
>> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
>> 5 files changed, 160 insertions(+), 19 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 81185d4311..456247af8f 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1449,12 +1449,37 @@
>> ##
>> { '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: uri of the source VM. Default port number is 0.
>> +#
>> +# @destination-uri: uri 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 7.1)
>> +#
>> +##
>> +{ '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)
>> #
>> @@ -1474,20 +1499,32 @@
>> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
>> +# 3. All boolean arguments default to false
>> +#
>> +# 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' } }
> Considering the existing migrate API from a QAPI design POV, I
> think there are several significant flaws with it
>
> The use of URIs is the big red flag. It is basically a data encoding
> scheme within a data encoding scheme. QEMU code should be able to
> directly work with the results from QAPI, without having todo a
> second level of parsing.
>
> URIs made sense in the context of HMP or the QemuOpts CLI, but do not
> make sense in QMP. We made a mistake in this respect when we first
> introduced QMP and implemented 'migrate'.
>
> If we going to extend the migrate API I think we should stop using URIs
> for the new fields, and instead define a QAPI discriminated union for
> the different data transport backends we offer.
>
> { 'enum': 'MigrateTransport',
> 'data': ['socket', 'exec'] }
>
> { 'union': 'MigrateAddress',
> 'base': { 'transport': 'MigrateTransport'},
> 'discriminator': 'transport',
> 'data': {
> 'socket': 'SocketAddress',
> 'exec': ['str'],
> }
>
> NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
> and 'fd' already. I'm fuzzy on best way to represent RDMA.
>
>
> IIUC, the desire of migration maintainers is that we can ultimately
> have multifd as the preferred, or even only, mechanism. Aside from
> the main outbound migration control channel, and the multifd
> data channels, IIUC we have a potential desire to have more channels
> for post-copy async requests.
>
> This all suggests to me a more general representation along the
> lines of:
>
> { 'enum': 'MigrateChannelType',
> 'data': ['control', 'data', 'async'] }
>
> { 'struct': 'MigrateChannel',
> 'data': {
> 'type': 'MigrateChannelType',
> 'src-addr': 'MigrateAddress',
> 'dst-addr': 'MigrateAddress',
> 'count': 'int',
> } }
>
> { 'comand': 'migrate',
> 'data': {
> '*uri': 'str'
> '*channels': ['MigrateChannel']
> }
> }
>
> With 'uri' and 'channels' being mutually exclusive here.
>
> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
> API wrt multifd - essentally the same data is now being set in two
> different places. IMHO, we should declare the 'multifd' capability
> and the 'multifd-chanels' parameter deprecated, since the information
> they provide is totally redundant, if you're giving an explicit list
> of channels to 'migrate'.
> Hi Daniel. Initially while brainstorming this idea for the first
time, we also came up with the same thought of depricating the migrate
API. But how will we achieve this now and how is it going to work. Is it
like we will be making migate V2 APIs initially, integrate it and then
depricate the old one? would be happy to get some pointers from your end.
> With regards,
> Daniel
With Regards,
Het Gala
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side
2022-07-26 11:20 ` Daniel P. Berrangé
@ 2022-07-28 15:05 ` Het Gala
0 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-28 15:05 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On 26/07/22 4:50 pm, Daniel P. Berrangé wrote:
> On Thu, Jul 21, 2022 at 07:56:16PM +0000, Het Gala wrote:
>> i) Modified the format of qemu monitor command: 'migrate-incoming' by adding
>> a list, each element in the list to open socket listeners with a given
>> number of multifd channels.
>>
>> ii) Qemu starts with -incoming flag defer and -multi-fd-incoming defer to
>> allow the modified 'migrate-incoming' command to be used.
>>
>> iii) Format for -multi-fd-incoming flag as a comma separated string has been
>> added with each substring containing listener socket address and number
>> of sockets to open.
>>
>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 79e00916a1..5555f0d2fd 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4479,6 +4479,24 @@ SRST
>> to issuing the migrate\_incoming to allow the migration to begin.
>> ERST
>>
>> +DEF("multi-fd-incoming", HAS_ARG, QEMU_OPTION_multi_fd_incoming, \
>> + "-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]\n" \
>> + "-multi-fd-incoming defer\n" \
>> + " wait for the URI to be specified via\n" \
>> + " multi_fd_migrate_incoming\n",
>> + QEMU_ARCH_ALL)
>> +SRST
>> +``-multi-fd-incoming tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]``
>> + Prepare for multi-fd incoming migration, with multi-fd listening sockets
>> + on that connection. Default number of multi-fd channels is 2.
>> +
>> +``-multi-fd-incoming defer``
>> + Wait for the URI to be specified via multi_fd_migrate\_incoming. The
>> + monitor can be used to change settings (such as migration parameters)
>> + prior to issuing the multi_fd_migrate\_incoming to allow the migration
>> + to begin.
>> +ERST
> We should not be adding any new -multi-fd-incoming CLI parameter at all.
> The CLI is so unsuitable for any complex configuration param and this
> is a prime example.
>
> If anything we should fully deprecate anything that is not '-incoming defer'
> such that we become 100% QMP/QAPI based for incoming migration config.
> Sure Daniel. We will depricate this -multi-fd-incoming defer flag and
only keep QMP/QAPI based migration config in the coming patchset series.
> With regards,
> Daniel
With Regards,
Het Gala
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair
2022-07-26 10:44 ` Daniel P. Berrangé
@ 2022-07-28 15:15 ` Het Gala
0 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-07-28 15:15 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On 26/07/22 4:14 pm, Daniel P. Berrangé wrote:
> In $SUBJECT s/multifd:/io:/ as this is not migration related.
>
> On Thu, Jul 21, 2022 at 07:56:18PM +0000, Het Gala wrote:
>> i) Binding of the socket to source ip address and port on the non-default
>> interface has been implemented for multi-FD connection, which was not
>> necessary earlier because the binding was on the default interface itself.
>>
>> ii) Created an end to end connection between all multi-FD source and
>> destination pairs.
>>
>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>> include/io/channel-socket.h | 44 ++++++++++++++++
>> include/qemu/sockets.h | 6 ++-
>> io/channel-socket.c | 93 ++++++++++++++++++++++++++--------
>> migration/socket.c | 4 +-
>> tests/unit/test-util-sockets.c | 16 +++---
>> util/qemu-sockets.c | 90 +++++++++++++++++++++++---------
>> 6 files changed, 196 insertions(+), 57 deletions(-)
>>
>> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
>> index 513c428fe4..8168866b06 100644
>> --- a/include/io/channel-socket.h
>> +++ b/include/io/channel-socket.h
>> @@ -211,6 +211,50 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
>> GMainContext *context);
>>
>>
>> +/**
>> + * qio_channel_socket_connect_all_sync:
> This needs to be called qio_channel_socket_connect_full_sync to
> match the naming conventions in use in IO code.
> Sorry Daniel, will definitely update this in the coming patchset for
sure.
>> + * @ioc: the socket channel object
>> + * @dst_addr: the destination address to connect to
>> + * @src_addr: the source address to be connected
> 'the optional source address to bind to'
> Sure, acknowledged.
>> + * @errp: pointer to a NULL-initialized error object
>> + *
>> + * Attempt to connect to the address @dst_addr with @src_addr.
> * Attempt to connect to the address @dst_addr. If @src_addr
> * is non-NULL, it will be bound to in order to control outbound
> * interface routing.
>
>
>> + * This method will run in the foreground so the caller will not
>> + * regain execution control until the connection is established or
>> + * an error occurs.
>> + */
>> +int qio_channel_socket_connect_all_sync(QIOChannelSocket *ioc,
>> + SocketAddress *dst_addr,
>> + SocketAddress *src_addr,
>> + Error **errp);
> Vertical mis-alignment of parameters
> Acknowledged.
>> +
>> +/**
>> + * qio_channel_socket_connect_all_async:
> Needs to be qio_channel_socket_connect_full_async
> Acknowledged. Sorry for such nit errors. Will update them in next
patchset
>> + * @ioc: the socket channel object
>> + * @dst_addr: the destination address to connect to
> @src_addr needs to be placed here...
> Acknowledged.
>> + * @callback: the function to invoke on completion
>> + * @opaque: user data to pass to @callback
>> + * @destroy: the function to free @opaque
>> + * @context: the context to run the async task. If %NULL, the default
>> + * context will be used.
>> + * @src_addr: the source address to be connected
> ...not here
>
> and same note about the docs comment
> Acknowledged
>> + *
>> + * Attempt to connect to the address @dst_addr with the @src_addr.
> Same note about the docs comment
> Acknowledged.
>
>> + * This method will run in the background so the caller will regain
>> + * execution control immediately. The function @callback
>> + * will be invoked on completion or failure. The @dst_addr
>> + * parameter will be copied, so may be freed as soon
>> + * as this function returns without waiting for completion.
>> + */
>> +void qio_channel_socket_connect_all_async(QIOChannelSocket *ioc,
>> + SocketAddress *dst_addr,
>> + QIOTaskFunc callback,
>> + gpointer opaque,
>> + GDestroyNotify destroy,
>> + GMainContext *context,
>> + SocketAddress *src_addr);
>> +
>> +
>> /**
>> * qio_channel_socket_get_local_address:
>> * @ioc: the socket channel object
>>
>>
>>
>>
>>
>> diff --git a/migration/socket.c b/migration/socket.c
>> index dab872a0fe..69fda774ba 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -57,8 +57,8 @@ int outgoing_param_total_multifds(void)
>> void socket_send_channel_create(QIOTaskFunc f, void *data)
>> {
>> QIOChannelSocket *sioc = qio_channel_socket_new();
>> - qio_channel_socket_connect_async(sioc, outgoing_args.saddr,
>> - f, data, NULL, NULL);
>> + qio_channel_socket_connect_all_async(sioc, outgoing_args.saddr,
>> + f, data, NULL, NULL, NULL);
>> }
> Don't change this call at all until the next patch which actually
> needs to pass a non-NULL parameter for src.
> Sure, acknowledged.
>> QIOChannel *socket_send_channel_create_sync(Error **errp)
>> diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
>> index 63909ccb2b..aa26630045 100644
>> --- a/tests/unit/test-util-sockets.c
>> +++ b/tests/unit/test-util-sockets.c
>> @@ -89,7 +89,7 @@ static void test_socket_fd_pass_name_good(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup(mon_fdname);
>>
>> - fd = socket_connect(&addr, &error_abort);
>> + fd = socket_connect(&addr, NULL, &error_abort);
>> g_assert_cmpint(fd, !=, -1);
>> g_assert_cmpint(fd, !=, mon_fd);
>> close(fd);
>> @@ -121,7 +121,7 @@ static void test_socket_fd_pass_name_bad(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup(mon_fdname);
>>
>> - fd = socket_connect(&addr, &err);
>> + fd = socket_connect(&addr, NULL, &err);
>> g_assert_cmpint(fd, ==, -1);
>> error_free_or_abort(&err);
>>
>> @@ -148,7 +148,7 @@ static void test_socket_fd_pass_name_nomon(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup("myfd");
>>
>> - fd = socket_connect(&addr, &err);
>> + fd = socket_connect(&addr, NULL, &err);
>> g_assert_cmpint(fd, ==, -1);
>> error_free_or_abort(&err);
>>
>> @@ -172,7 +172,7 @@ static void test_socket_fd_pass_num_good(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup_printf("%d", sfd);
>>
>> - fd = socket_connect(&addr, &error_abort);
>> + fd = socket_connect(&addr, NULL, &error_abort);
>> g_assert_cmpint(fd, ==, sfd);
>>
>> fd = socket_listen(&addr, 1, &error_abort);
>> @@ -194,7 +194,7 @@ static void test_socket_fd_pass_num_bad(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup_printf("%d", sfd);
>>
>> - fd = socket_connect(&addr, &err);
>> + fd = socket_connect(&addr, NULL, &err);
>> g_assert_cmpint(fd, ==, -1);
>> error_free_or_abort(&err);
>>
>> @@ -217,7 +217,7 @@ static void test_socket_fd_pass_num_nocli(void)
>> addr.type = SOCKET_ADDRESS_TYPE_FD;
>> addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO);
>>
>> - fd = socket_connect(&addr, &err);
>> + fd = socket_connect(&addr, NULL, &err);
>> g_assert_cmpint(fd, ==, -1);
>> error_free_or_abort(&err);
>>
>> @@ -246,10 +246,10 @@ static gpointer unix_client_thread_func(gpointer user_data)
>>
>> for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
>> if (row->expect_connect[i]) {
>> - fd = socket_connect(row->client[i], &error_abort);
>> + fd = socket_connect(row->client[i], NULL, &error_abort);
>> g_assert_cmpint(fd, >=, 0);
>> } else {
>> - fd = socket_connect(row->client[i], &err);
>> + fd = socket_connect(row->client[i], NULL, &err);
>> g_assert_cmpint(fd, ==, -1);
>> error_free_or_abort(&err);
>> }
> I'd expect something added to the test suite to exercise the new
> codepath. Obviously we'd be limted to dealing with 127.0.0.1,
> but it can at least run the code paths.
> Sure Daniel. Will add some test cases from the coming v3 patchset
series.
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 13b5b197f9..491e2f2bc9 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -358,8 +358,10 @@ listen_ok:
>> ((rc) == -EINPROGRESS)
>> #endif
>>
>> -static int inet_connect_addr(const InetSocketAddress *saddr,
>> - struct addrinfo *addr, Error **errp)
>> +static int inet_connect_addr(const InetSocketAddress *dst_addr,
>> + const InetSocketAddress *src_addr,
>> + struct addrinfo *addr, struct addrinfo *saddr,
>> + Error **errp)
>> {
>> int sock, rc;
>>
>> @@ -371,8 +373,28 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
>> }
>> socket_set_fast_reuse(sock);
>>
>> + /* to bind the socket if src_addr is available */
>> +
>> + if (src_addr) {
>> + struct sockaddr_in servaddr;
>> +
>> + /* bind to a specific interface in the internet domain */
>> + /* to make sure the sin_zero filed is cleared */
>> + memset(&servaddr, 0, sizeof(servaddr));
>> +
>> + servaddr.sin_family = AF_INET;
>> + servaddr.sin_addr.s_addr = inet_addr(src_addr->host);
> My feedback from the previous posting has been ignored. This code is
> broken for IPv6. Never call the IPv4-only APIs, getaddrinfo is the
> only way to get a 'struct sockaddr *' in a protocol portable manner.
> Sorry Daniel, my appologies. I certainly misunderstood your point in
the previous patchset. I thought this post was in sync with the
ai_family check inet_connect_saddr function, and we wanted the src_addr
to be called for getaddrinfo function in that context. But, I get your
point now. I will surely update this in the v3 patchset series.
>> + servaddr.sin_port = 0;
>> +
>> + if (bind(sock, (struct sockaddr *)&servaddr, sizeof(servaddr)) < 0) {
>> + error_setg_errno(errp, errno, "Failed to bind socket");
>> + return -1;
>> + }
>> + }
>> +
>> /* connect to peer */
>> do {
>> +
> Arbitrary whitespace change should be removed
>
>> rc = 0;
>> if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
>> rc = -errno;
>> @@ -380,8 +402,14 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
>> @@ -446,41 +474,55 @@ static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
>> *
>> * Returns: -1 on error, file descriptor on success.
>> */
>> -int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>> +int inet_connect_saddr(InetSocketAddress *dst_addr,
>> + InetSocketAddress *src_addr, Error **errp)
>> {
>> Error *local_err = NULL;
>> - struct addrinfo *res, *e;
>> + struct addrinfo *res_d, *res_s, *e, *x;
>> int sock = -1;
>>
>> - res = inet_parse_connect_saddr(saddr, errp);
>> - if (!res) {
>> + res_d = inet_parse_connect_saddr(dst_addr, errp);
>> + if (!res_d) {
>> return -1;
>> }
>>
>> - for (e = res; e != NULL; e = e->ai_next) {
>> + if (src_addr) {
>> + res_s = inet_parse_connect_saddr(src_addr, errp);
>> + if (!res_s) {
>> + return -1;
>> + }
>> + }
>> +
>> + for (e = res_d; e != NULL; e = e->ai_next) {
>> error_free(local_err);
>> local_err = NULL;
>>
>> #ifdef HAVE_IPPROTO_MPTCP
>> - if (saddr->has_mptcp && saddr->mptcp) {
>> + if (dst_addr->has_mptcp && dst_addr->mptcp) {
>> e->ai_protocol = IPPROTO_MPTCP;
>> }
>> #endif
>> + for (x = res_s; x != NULL; x = x->ai_next) {
>> + if (!src_addr || e->ai_family == x->ai_family) {
>>
>> - sock = inet_connect_addr(saddr, e, &local_err);
>> - if (sock >= 0) {
>> - break;
>> + sock = inet_connect_addr(dst_addr, src_addr,
>> + e, x, &local_err);
>> + if (sock >= 0) {
>> + break;
>> + }
>> + }
>> }
>> }
> If the ai_family for the src is different from ai_family for
> the dst, this loop will never call inet_connect_addr at all,
> and leave local_err unset, and so the error_propagate call
> below will have no error message to propagate.
> Yes, you are right, so we need to check and have a error placed here,
in-case if it never calls inet_connect_addr func, then we should print
an error statement there right. Thankyou Daniel for pointing this out.
>>
>> - freeaddrinfo(res);
>> + freeaddrinfo(res_d);
>> + freeaddrinfo(res_s);
>>
>> if (sock < 0) {
>> error_propagate(errp, local_err);
>> return sock;
>> }
>>
>> - if (saddr->keep_alive) {
>> + if (dst_addr->keep_alive) {
>> int val = 1;
>> int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
>> &val, sizeof(val));
>> @@ -506,7 +548,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
>> Error *err = NULL;
>>
>> /* lookup peer addr */
>> - memset(&ai,0, sizeof(ai));
>> + memset(&ai,0,sizeof(ai));
> Unrelated whitespace change.
> Acknowledged.
>> ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
>> ai.ai_family = inet_ai_family_from_address(sraddr, &err);
>> ai.ai_socktype = SOCK_DGRAM;
>> @@ -727,7 +769,7 @@ int inet_connect(const char *str, Error **errp)
>> InetSocketAddress *addr = g_new(InetSocketAddress, 1);
>>
>> if (!inet_parse(addr, str, errp)) {
>> - sock = inet_connect_saddr(addr, errp);
>> + sock = inet_connect_saddr(addr, NULL, errp);
>> }
>> qapi_free_InetSocketAddress(addr);
>> return sock;
> With regards,
> Daniel
With regards,
Het Gala
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-07-28 15:02 ` Het Gala
@ 2022-08-02 7:53 ` Markus Armbruster
2022-08-08 6:11 ` Het Gala
2022-11-21 12:26 ` Juan Quintela
2022-08-08 9:29 ` Daniel P. Berrangé
1 sibling, 2 replies; 22+ messages in thread
From: Markus Armbruster @ 2022-08-02 7:53 UTC (permalink / raw)
To: Het Gala
Cc: Daniel P. Berrangé,
qemu-devel, quintela, dgilbert, pbonzini, eblake, prerna.saxena,
Manish Mishra
Het Gala <het.gala@nutanix.com> writes:
> On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
>> On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
>>> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
>>> each element in the list consisting of multifd connection parameters: source
>>> uri, destination uri and of the number of multifd channels between each pair.
>>>
>>> ii) Information of all multifd connection parameters' list and length of the
>>> list is stored in 'OutgoingMigrateParams' struct.
>>>
>>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
>>> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>>> migration/socket.h | 19 +++++++++++++-
>>> monitor/hmp-cmds.c | 1 +
>>> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
>>> 5 files changed, 160 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 81185d4311..456247af8f 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1449,12 +1449,37 @@
>>> ##
>>> { '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: uri of the source VM. Default port number is 0.
>>> +#
>>> +# @destination-uri: uri 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 7.1)
>>> +#
>>> +##
>>> +{ '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)
>>> #
>>> @@ -1474,20 +1499,32 @@
>>> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
>>> +# 3. All boolean arguments default to false
>>> +#
>>> +# 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' } }
>>
>> Considering the existing migrate API from a QAPI design POV, I
>> think there are several significant flaws with it
>>
>> The use of URIs is the big red flag. It is basically a data encoding
>> scheme within a data encoding scheme. QEMU code should be able to
>> directly work with the results from QAPI, without having todo a
>> second level of parsing.
Concur.
>> URIs made sense in the context of HMP or the QemuOpts CLI, but do not
>> make sense in QMP. We made a mistake in this respect when we first
>> introduced QMP and implemented 'migrate'.
>>
>> If we going to extend the migrate API I think we should stop using URIs
>> for the new fields, and instead define a QAPI discriminated union for
>> the different data transport backends we offer.
>>
>> { 'enum': 'MigrateTransport',
>> 'data': ['socket', 'exec'] }
>>
>> { 'union': 'MigrateAddress',
>> 'base': { 'transport': 'MigrateTransport'},
>> 'discriminator': 'transport',
>> 'data': {
>> 'socket': 'SocketAddress',
>> 'exec': ['str'],
>> }
>>
>> NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
>> and 'fd' already. I'm fuzzy on best way to represent RDMA.
>>
>>
>> IIUC, the desire of migration maintainers is that we can ultimately
>> have multifd as the preferred, or even only, mechanism. Aside from
>> the main outbound migration control channel, and the multifd
>> data channels, IIUC we have a potential desire to have more channels
>> for post-copy async requests.
>>
>> This all suggests to me a more general representation along the
>> lines of:
>>
>> { 'enum': 'MigrateChannelType',
>> 'data': ['control', 'data', 'async'] }
>>
>> { 'struct': 'MigrateChannel',
>> 'data': {
>> 'type': 'MigrateChannelType',
>> 'src-addr': 'MigrateAddress',
>> 'dst-addr': 'MigrateAddress',
>> 'count': 'int',
>> } }
>>
>> { 'comand': 'migrate',
>> 'data': {
>> '*uri': 'str'
>> '*channels': ['MigrateChannel']
>> }
>> }
>>
>> With 'uri' and 'channels' being mutually exclusive here.
>>
>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
>> API wrt multifd - essentally the same data is now being set in two
>> different places. IMHO, we should declare the 'multifd' capability
>> and the 'multifd-chanels' parameter deprecated, since the information
>> they provide is totally redundant, if you're giving an explicit list
>> of channels to 'migrate'.
>
> Hi Daniel. Initially while brainstorming this idea for the first time, we also came up with the same thought of depricating the migrate
> API. But how will we achieve this now and how is it going to work. Is it like we will be making migate V2 APIs initially, integrate it and then
> depricate the old one? would be happy to get some pointers from your end.
Migration maintainers, please advise.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-08-02 7:53 ` Markus Armbruster
@ 2022-08-08 6:11 ` Het Gala
2022-08-08 9:29 ` Daniel P. Berrangé
2022-08-29 4:34 ` Het Gala
2022-11-21 12:26 ` Juan Quintela
1 sibling, 2 replies; 22+ messages in thread
From: Het Gala @ 2022-08-08 6:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé,
qemu-devel, quintela, dgilbert, pbonzini, eblake, prerna.saxena,
Manish Mishra
[-- Attachment #1: Type: text/plain, Size: 7619 bytes --]
On 02/08/22 1:23 pm, Markus Armbruster wrote:
> Het Gala<het.gala@nutanix.com> writes:
>
>> On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
>>> On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
>>>> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
>>>> each element in the list consisting of multifd connection parameters: source
>>>> uri, destination uri and of the number of multifd channels between each pair.
>>>>
>>>> ii) Information of all multifd connection parameters' list and length of the
>>>> list is stored in 'OutgoingMigrateParams' struct.
>>>>
>>>> Suggested-by: Manish Mishra<manish.mishra@nutanix.com>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> ---
>>>> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
>>>> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>>>> migration/socket.h | 19 +++++++++++++-
>>>> monitor/hmp-cmds.c | 1 +
>>>> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
>>>> 5 files changed, 160 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 81185d4311..456247af8f 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1449,12 +1449,37 @@
>>>> ##
>>>> { '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: uri of the source VM. Default port number is 0.
>>>> +#
>>>> +# @destination-uri: uri 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 7.1)
>>>> +#
>>>> +##
>>>> +{ '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)
>>>> #
>>>> @@ -1474,20 +1499,32 @@
>>>> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
>>>> +# 3. All boolean arguments default to false
>>>> +#
>>>> +# 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' } }
>>> Considering the existing migrate API from a QAPI design POV, I
>>> think there are several significant flaws with it
>>>
>>> The use of URIs is the big red flag. It is basically a data encoding
>>> scheme within a data encoding scheme. QEMU code should be able to
>>> directly work with the results from QAPI, without having todo a
>>> second level of parsing.
> Concur.
>
>>> URIs made sense in the context of HMP or the QemuOpts CLI, but do not
>>> make sense in QMP. We made a mistake in this respect when we first
>>> introduced QMP and implemented 'migrate'.
>>>
>>> If we going to extend the migrate API I think we should stop using URIs
>>> for the new fields, and instead define a QAPI discriminated union for
>>> the different data transport backends we offer.
>>>
>>> { 'enum': 'MigrateTransport',
>>> 'data': ['socket', 'exec'] }
>>>
>>> { 'union': 'MigrateAddress',
>>> 'base': { 'transport': 'MigrateTransport'},
>>> 'discriminator': 'transport',
>>> 'data': {
>>> 'socket': 'SocketAddress',
>>> 'exec': ['str'],
>>> }
>>>
>>> NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
>>> and 'fd' already. I'm fuzzy on best way to represent RDMA.
>>>
>>>
>>> IIUC, the desire of migration maintainers is that we can ultimately
>>> have multifd as the preferred, or even only, mechanism. Aside from
>>> the main outbound migration control channel, and the multifd
>>> data channels, IIUC we have a potential desire to have more channels
>>> for post-copy async requests.
>>>
>>> This all suggests to me a more general representation along the
>>> lines of:
>>>
>>> { 'enum': 'MigrateChannelType',
>>> 'data': ['control', 'data', 'async'] }
>>>
>>> { 'struct': 'MigrateChannel',
>>> 'data': {
>>> 'type': 'MigrateChannelType',
>>> 'src-addr': 'MigrateAddress',
>>> 'dst-addr': 'MigrateAddress',
>>> 'count': 'int',
>>> } }
>>>
>>> { 'comand': 'migrate',
>>> 'data': {
>>> '*uri': 'str'
>>> '*channels': ['MigrateChannel']
>>> }
>>> }
>>>
>>> With 'uri' and 'channels' being mutually exclusive here.
>>>
>>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
>>> API wrt multifd - essentally the same data is now being set in two
>>> different places. IMHO, we should declare the 'multifd' capability
>>> and the 'multifd-chanels' parameter deprecated, since the information
>>> they provide is totally redundant, if you're giving an explicit list
>>> of channels to 'migrate'.
>> Hi Daniel. Initially while brainstorming this idea for the first time, we also came up with the same thought of depricating the migrate
>> API. But how will we achieve this now and how is it going to work. Is it like we will be making migate V2 APIs initially, integrate it and then
>> depricate the old one? would be happy to get some pointers from your end.
> Migration maintainers, please advise.
Hi Daniel and other migraton maintainers: Dr. David and Juan, what is your opinion on this. And how can we go forward implementing this. Some pointers and ideas from your end would be helpful too.
Regards,
Het Gala
[-- Attachment #2: Type: text/html, Size: 8389 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-07-28 15:02 ` Het Gala
2022-08-02 7:53 ` Markus Armbruster
@ 2022-08-08 9:29 ` Daniel P. Berrangé
1 sibling, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-08-08 9:29 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, quintela, dgilbert, pbonzini, armbru, eblake,
prerna.saxena, Manish Mishra
On Thu, Jul 28, 2022 at 08:32:39PM +0530, Het Gala wrote:
>
> On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
> > On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
> > > i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
> > > each element in the list consisting of multifd connection parameters: source
> > > uri, destination uri and of the number of multifd channels between each pair.
> > >
> > > ii) Information of all multifd connection parameters' list and length of the
> > > list is stored in 'OutgoingMigrateParams' struct.
> > >
> > > Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > > migration/migration.c | 52 +++++++++++++++++++++++++++++--------
> > > migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
> > > migration/socket.h | 19 +++++++++++++-
> > > monitor/hmp-cmds.c | 1 +
> > > qapi/migration.json | 47 +++++++++++++++++++++++++++++----
> > > 5 files changed, 160 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index 81185d4311..456247af8f 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -1449,12 +1449,37 @@
> > > ##
> > > { '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: uri of the source VM. Default port number is 0.
> > > +#
> > > +# @destination-uri: uri 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 7.1)
> > > +#
> > > +##
> > > +{ '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)
> > > #
> > > @@ -1474,20 +1499,32 @@
> > > # 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. The user Monitor's "detach" argument is invalid in QMP and should not
> > > +# 3. All boolean arguments default to false
> > > +#
> > > +# 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' } }
> > Considering the existing migrate API from a QAPI design POV, I
> > think there are several significant flaws with it
> >
> > The use of URIs is the big red flag. It is basically a data encoding
> > scheme within a data encoding scheme. QEMU code should be able to
> > directly work with the results from QAPI, without having todo a
> > second level of parsing.
> >
> > URIs made sense in the context of HMP or the QemuOpts CLI, but do not
> > make sense in QMP. We made a mistake in this respect when we first
> > introduced QMP and implemented 'migrate'.
> >
> > If we going to extend the migrate API I think we should stop using URIs
> > for the new fields, and instead define a QAPI discriminated union for
> > the different data transport backends we offer.
> >
> > { 'enum': 'MigrateTransport',
> > 'data': ['socket', 'exec'] }
> >
> > { 'union': 'MigrateAddress',
> > 'base': { 'transport': 'MigrateTransport'},
> > 'discriminator': 'transport',
> > 'data': {
> > 'socket': 'SocketAddress',
> > 'exec': ['str'],
> > }
> >
> > NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
> > and 'fd' already. I'm fuzzy on best way to represent RDMA.
> >
> >
> > IIUC, the desire of migration maintainers is that we can ultimately
> > have multifd as the preferred, or even only, mechanism. Aside from
> > the main outbound migration control channel, and the multifd
> > data channels, IIUC we have a potential desire to have more channels
> > for post-copy async requests.
> >
> > This all suggests to me a more general representation along the
> > lines of:
> >
> > { 'enum': 'MigrateChannelType',
> > 'data': ['control', 'data', 'async'] }
> >
> > { 'struct': 'MigrateChannel',
> > 'data': {
> > 'type': 'MigrateChannelType',
> > 'src-addr': 'MigrateAddress',
> > 'dst-addr': 'MigrateAddress',
> > 'count': 'int',
> > } }
> >
> > { 'comand': 'migrate',
> > 'data': {
> > '*uri': 'str'
> > '*channels': ['MigrateChannel']
> > }
> > }
> >
> > With 'uri' and 'channels' being mutually exclusive here.
> >
> > This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
> > API wrt multifd - essentally the same data is now being set in two
> > different places. IMHO, we should declare the 'multifd' capability
> > and the 'multifd-chanels' parameter deprecated, since the information
> > they provide is totally redundant, if you're giving an explicit list
> > of channels to 'migrate'.
> > Hi Daniel. Initially while brainstorming this idea for the first time, we
> also came up with the same thought of depricating the migrate API. But how
> will we achieve this now and how is it going to work. Is it like we will be
> making migate V2 APIs initially, integrate it and then depricate the old
> one? would be happy to get some pointers from your end.
I don't think we need to replace 'migrate' with 'migrate2', because our
QAPI deprecation & feature addition policy lets us evolve the existing
commands over time.
We currently have
{ 'comand': 'migrate',
'data': {
'uri': 'str'
}
}
Our ABI policy lets us make 'uri' optional, and as well as adding new
optional fields:
{ 'comand': 'migrate',
'data': {
'*uri': 'str'
'*channels': ['MigrateChannel']
}
}
At some point we can choose to deprecate 'uri' entirely and just ask
people to use a single element 'channels' array. After having 'uri'
deprecated for a while we can delete it and make 'channels' mandatory
{ 'comand': 'migrate',
'data': {
'channels': ['MigrateChannel']
}
}
This gives us the same end point as if we had added
{ 'comand': 'migrate2',
'data': {
'channels': ['MigrateChannel']
}
}
and deprecated &deleted 'migrate' entirely, but without leaving us with
the ugly naming.
FWIW, if I was going to majorly refactor migration QAPI design, I would
also seek to get rid of 'migrate-set-capability' entirely and just
make it a parameter to 'migrate', and likewise for the same for any of
'migrate-set-parameter' that are only used as upfront tunables. We
should only need 'migrate-set-parameter' for runtime tunables that need
setting /after/ migration has already started. I didn't mention this
before though, as I didn't want to impose a big chunk of extra work
on this feature of yours, as it can be done separately at any time.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-08-08 6:11 ` Het Gala
@ 2022-08-08 9:29 ` Daniel P. Berrangé
2022-08-29 4:34 ` Het Gala
1 sibling, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-08-08 9:29 UTC (permalink / raw)
To: Het Gala
Cc: Markus Armbruster, qemu-devel, quintela, dgilbert, pbonzini,
eblake, prerna.saxena, Manish Mishra
On Mon, Aug 08, 2022 at 11:41:21AM +0530, Het Gala wrote:
>
> On 02/08/22 1:23 pm, Markus Armbruster wrote:
> > Het Gala<het.gala@nutanix.com> writes:
> >
> > > On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
> > > > On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
> > > > > i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
> > > > > each element in the list consisting of multifd connection parameters: source
> > > > > uri, destination uri and of the number of multifd channels between each pair.
> > > > >
> > > > > ii) Information of all multifd connection parameters' list and length of the
> > > > > list is stored in 'OutgoingMigrateParams' struct.
> > > > >
> > > > > Suggested-by: Manish Mishra<manish.mishra@nutanix.com>
> > > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > > ---
> > > > > migration/migration.c | 52 +++++++++++++++++++++++++++++--------
> > > > > migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
> > > > > migration/socket.h | 19 +++++++++++++-
> > > > > monitor/hmp-cmds.c | 1 +
> > > > > qapi/migration.json | 47 +++++++++++++++++++++++++++++----
> > > > > 5 files changed, 160 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > > index 81185d4311..456247af8f 100644
> > > > > --- a/qapi/migration.json
> > > > > +++ b/qapi/migration.json
> > > > > @@ -1449,12 +1449,37 @@
> > > > > ##
> > > > > { '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: uri of the source VM. Default port number is 0.
> > > > > +#
> > > > > +# @destination-uri: uri 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 7.1)
> > > > > +#
> > > > > +##
> > > > > +{ '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)
> > > > > #
> > > > > @@ -1474,20 +1499,32 @@
> > > > > # 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. The user Monitor's "detach" argument is invalid in QMP and should not
> > > > > +# 3. All boolean arguments default to false
> > > > > +#
> > > > > +# 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' } }
> > > > Considering the existing migrate API from a QAPI design POV, I
> > > > think there are several significant flaws with it
> > > >
> > > > The use of URIs is the big red flag. It is basically a data encoding
> > > > scheme within a data encoding scheme. QEMU code should be able to
> > > > directly work with the results from QAPI, without having todo a
> > > > second level of parsing.
> > Concur.
> >
> > > > URIs made sense in the context of HMP or the QemuOpts CLI, but do not
> > > > make sense in QMP. We made a mistake in this respect when we first
> > > > introduced QMP and implemented 'migrate'.
> > > >
> > > > If we going to extend the migrate API I think we should stop using URIs
> > > > for the new fields, and instead define a QAPI discriminated union for
> > > > the different data transport backends we offer.
> > > >
> > > > { 'enum': 'MigrateTransport',
> > > > 'data': ['socket', 'exec'] }
> > > >
> > > > { 'union': 'MigrateAddress',
> > > > 'base': { 'transport': 'MigrateTransport'},
> > > > 'discriminator': 'transport',
> > > > 'data': {
> > > > 'socket': 'SocketAddress',
> > > > 'exec': ['str'],
> > > > }
> > > >
> > > > NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
> > > > and 'fd' already. I'm fuzzy on best way to represent RDMA.
> > > >
> > > >
> > > > IIUC, the desire of migration maintainers is that we can ultimately
> > > > have multifd as the preferred, or even only, mechanism. Aside from
> > > > the main outbound migration control channel, and the multifd
> > > > data channels, IIUC we have a potential desire to have more channels
> > > > for post-copy async requests.
> > > >
> > > > This all suggests to me a more general representation along the
> > > > lines of:
> > > >
> > > > { 'enum': 'MigrateChannelType',
> > > > 'data': ['control', 'data', 'async'] }
> > > >
> > > > { 'struct': 'MigrateChannel',
> > > > 'data': {
> > > > 'type': 'MigrateChannelType',
> > > > 'src-addr': 'MigrateAddress',
> > > > 'dst-addr': 'MigrateAddress',
> > > > 'count': 'int',
> > > > } }
> > > >
> > > > { 'comand': 'migrate',
> > > > 'data': {
> > > > '*uri': 'str'
> > > > '*channels': ['MigrateChannel']
> > > > }
> > > > }
> > > >
> > > > With 'uri' and 'channels' being mutually exclusive here.
> > > >
> > > > This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
> > > > API wrt multifd - essentally the same data is now being set in two
> > > > different places. IMHO, we should declare the 'multifd' capability
> > > > and the 'multifd-chanels' parameter deprecated, since the information
> > > > they provide is totally redundant, if you're giving an explicit list
> > > > of channels to 'migrate'.
> > > Hi Daniel. Initially while brainstorming this idea for the first time, we also came up with the same thought of depricating the migrate
> > > API. But how will we achieve this now and how is it going to work. Is it like we will be making migate V2 APIs initially, integrate it and then
> > > depricate the old one? would be happy to get some pointers from your end.
> > Migration maintainers, please advise.
>
> Hi Daniel and other migraton maintainers: Dr. David and Juan, what
> is your opinion on this. And how can we go forward implementing
> this. Some pointers and ideas from your end would be helpful too.
Note, i'm not a migration maintainer, just offering advice from the
sidelines :-)
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-08-08 6:11 ` Het Gala
2022-08-08 9:29 ` Daniel P. Berrangé
@ 2022-08-29 4:34 ` Het Gala
2022-09-21 10:08 ` Het Gala
1 sibling, 1 reply; 22+ messages in thread
From: Het Gala @ 2022-08-29 4:34 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé,
qemu-devel, quintela, dgilbert, pbonzini, eblake, prerna.saxena,
Manish Mishra
[-- Attachment #1: Type: text/plain, Size: 8044 bytes --]
On 08/08/22 11:41 am, Het Gala wrote:
>
>
> On 02/08/22 1:23 pm, Markus Armbruster wrote:
>> Het Gala<het.gala@nutanix.com> writes:
>>
>>> On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
>>>> On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
>>>>> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
>>>>> each element in the list consisting of multifd connection parameters: source
>>>>> uri, destination uri and of the number of multifd channels between each pair.
>>>>>
>>>>> ii) Information of all multifd connection parameters' list and length of the
>>>>> list is stored in 'OutgoingMigrateParams' struct.
>>>>>
>>>>> Suggested-by: Manish Mishra<manish.mishra@nutanix.com>
>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>>> ---
>>>>> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
>>>>> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>>>>> migration/socket.h | 19 +++++++++++++-
>>>>> monitor/hmp-cmds.c | 1 +
>>>>> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
>>>>> 5 files changed, 160 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index 81185d4311..456247af8f 100644
>>>>> --- a/qapi/migration.json
>>>>> +++ b/qapi/migration.json
>>>>> @@ -1449,12 +1449,37 @@
>>>>> ##
>>>>> { '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: uri of the source VM. Default port number is 0.
>>>>> +#
>>>>> +# @destination-uri: uri 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 7.1)
>>>>> +#
>>>>> +##
>>>>> +{ '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)
>>>>> #
>>>>> @@ -1474,20 +1499,32 @@
>>>>> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
>>>>> +# 3. All boolean arguments default to false
>>>>> +#
>>>>> +# 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' } }
>>>> Considering the existing migrate API from a QAPI design POV, I
>>>> think there are several significant flaws with it
>>>>
>>>> The use of URIs is the big red flag. It is basically a data encoding
>>>> scheme within a data encoding scheme. QEMU code should be able to
>>>> directly work with the results from QAPI, without having todo a
>>>> second level of parsing.
>> Concur.
>>
>>>> URIs made sense in the context of HMP or the QemuOpts CLI, but do not
>>>> make sense in QMP. We made a mistake in this respect when we first
>>>> introduced QMP and implemented 'migrate'.
>>>>
>>>> If we going to extend the migrate API I think we should stop using URIs
>>>> for the new fields, and instead define a QAPI discriminated union for
>>>> the different data transport backends we offer.
>>>>
>>>> { 'enum': 'MigrateTransport',
>>>> 'data': ['socket', 'exec'] }
>>>>
>>>> { 'union': 'MigrateAddress',
>>>> 'base': { 'transport': 'MigrateTransport'},
>>>> 'discriminator': 'transport',
>>>> 'data': {
>>>> 'socket': 'SocketAddress',
>>>> 'exec': ['str'],
>>>> }
>>>>
>>>> NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
>>>> and 'fd' already. I'm fuzzy on best way to represent RDMA.
>>>>
>>>>
>>>> IIUC, the desire of migration maintainers is that we can ultimately
>>>> have multifd as the preferred, or even only, mechanism. Aside from
>>>> the main outbound migration control channel, and the multifd
>>>> data channels, IIUC we have a potential desire to have more channels
>>>> for post-copy async requests.
>>>>
>>>> This all suggests to me a more general representation along the
>>>> lines of:
>>>>
>>>> { 'enum': 'MigrateChannelType',
>>>> 'data': ['control', 'data', 'async'] }
>>>>
>>>> { 'struct': 'MigrateChannel',
>>>> 'data': {
>>>> 'type': 'MigrateChannelType',
>>>> 'src-addr': 'MigrateAddress',
>>>> 'dst-addr': 'MigrateAddress',
>>>> 'count': 'int',
>>>> } }
>>>>
>>>> { 'comand': 'migrate',
>>>> 'data': {
>>>> '*uri': 'str'
>>>> '*channels': ['MigrateChannel']
>>>> }
>>>> }
>>>>
>>>> With 'uri' and 'channels' being mutually exclusive here.
>>>>
>>>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
>>>> API wrt multifd - essentally the same data is now being set in two
>>>> different places. IMHO, we should declare the 'multifd' capability
>>>> and the 'multifd-chanels' parameter deprecated, since the information
>>>> they provide is totally redundant, if you're giving an explicit list
>>>> of channels to 'migrate'.
>>> Hi Daniel. Initially while brainstorming this idea for the first time, we also came up with the same thought of depricating the migrate
>>> API. But how will we achieve this now and how is it going to work. Is it like we will be making migate V2 APIs initially, integrate it and then
>>> depricate the old one? would be happy to get some pointers from your end.
>> Migration maintainers, please advise.
> Hi Daniel and other migraton maintainers: Dr. David and Juan, what is your opinion on this. And how can we go forward implementing this. Some pointers and ideas from your end would be helpful too.
> Regards,
> Het Gala
Just a gentle reminder Dr. David and Juan. Daniel already expressed his
opinion to refactor the QAPI design for multifd. Your inputs and advice
will also be very valuable to us. Thankyou.
Regards,
Het Gala
[-- Attachment #2: Type: text/html, Size: 8976 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-08-29 4:34 ` Het Gala
@ 2022-09-21 10:08 ` Het Gala
0 siblings, 0 replies; 22+ messages in thread
From: Het Gala @ 2022-09-21 10:08 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé,
qemu-devel, quintela, dgilbert, pbonzini, eblake, prerna.saxena,
Manish Mishra
[-- Attachment #1: Type: text/plain, Size: 8686 bytes --]
On 29/08/22 10:04 am, Het Gala wrote:
>
>
> On 08/08/22 11:41 am, Het Gala wrote:
>>
>>
>> On 02/08/22 1:23 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com> writes:
>>>
>>>> On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:
>>>>> On Thu, Jul 21, 2022 at 07:56:15PM +0000, Het Gala wrote:
>>>>>> i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
>>>>>> each element in the list consisting of multifd connection parameters: source
>>>>>> uri, destination uri and of the number of multifd channels between each pair.
>>>>>>
>>>>>> ii) Information of all multifd connection parameters' list and length of the
>>>>>> list is stored in 'OutgoingMigrateParams' struct.
>>>>>>
>>>>>> Suggested-by: Manish Mishra<manish.mishra@nutanix.com>
>>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>>>> ---
>>>>>> migration/migration.c | 52 +++++++++++++++++++++++++++++--------
>>>>>> migration/socket.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>>>>>> migration/socket.h | 19 +++++++++++++-
>>>>>> monitor/hmp-cmds.c | 1 +
>>>>>> qapi/migration.json | 47 +++++++++++++++++++++++++++++----
>>>>>> 5 files changed, 160 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>>> index 81185d4311..456247af8f 100644
>>>>>> --- a/qapi/migration.json
>>>>>> +++ b/qapi/migration.json
>>>>>> @@ -1449,12 +1449,37 @@
>>>>>> ##
>>>>>> { '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: uri of the source VM. Default port number is 0.
>>>>>> +#
>>>>>> +# @destination-uri: uri 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 7.1)
>>>>>> +#
>>>>>> +##
>>>>>> +{ '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)
>>>>>> #
>>>>>> @@ -1474,20 +1499,32 @@
>>>>>> # 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. The user Monitor's "detach" argument is invalid in QMP and should not
>>>>>> +# 3. All boolean arguments default to false
>>>>>> +#
>>>>>> +# 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' } }
>>>>> Considering the existing migrate API from a QAPI design POV, I
>>>>> think there are several significant flaws with it
>>>>>
>>>>> The use of URIs is the big red flag. It is basically a data encoding
>>>>> scheme within a data encoding scheme. QEMU code should be able to
>>>>> directly work with the results from QAPI, without having todo a
>>>>> second level of parsing.
>>> Concur.
>>>
>>>>> URIs made sense in the context of HMP or the QemuOpts CLI, but do not
>>>>> make sense in QMP. We made a mistake in this respect when we first
>>>>> introduced QMP and implemented 'migrate'.
>>>>>
>>>>> If we going to extend the migrate API I think we should stop using URIs
>>>>> for the new fields, and instead define a QAPI discriminated union for
>>>>> the different data transport backends we offer.
>>>>>
>>>>> { 'enum': 'MigrateTransport',
>>>>> 'data': ['socket', 'exec'] }
>>>>>
>>>>> { 'union': 'MigrateAddress',
>>>>> 'base': { 'transport': 'MigrateTransport'},
>>>>> 'discriminator': 'transport',
>>>>> 'data': {
>>>>> 'socket': 'SocketAddress',
>>>>> 'exec': ['str'],
>>>>> }
>>>>>
>>>>> NB, 'socket' should be able to cover all of 'tcp', 'unix', 'vsock'
>>>>> and 'fd' already. I'm fuzzy on best way to represent RDMA.
>>>>>
>>>>>
>>>>> IIUC, the desire of migration maintainers is that we can ultimately
>>>>> have multifd as the preferred, or even only, mechanism. Aside from
>>>>> the main outbound migration control channel, and the multifd
>>>>> data channels, IIUC we have a potential desire to have more channels
>>>>> for post-copy async requests.
>>>>>
>>>>> This all suggests to me a more general representation along the
>>>>> lines of:
>>>>>
>>>>> { 'enum': 'MigrateChannelType',
>>>>> 'data': ['control', 'data', 'async'] }
>>>>>
>>>>> { 'struct': 'MigrateChannel',
>>>>> 'data': {
>>>>> 'type': 'MigrateChannelType',
>>>>> 'src-addr': 'MigrateAddress',
>>>>> 'dst-addr': 'MigrateAddress',
>>>>> 'count': 'int',
>>>>> } }
>>>>>
>>>>> { 'comand': 'migrate',
>>>>> 'data': {
>>>>> '*uri': 'str'
>>>>> '*channels': ['MigrateChannel']
>>>>> }
>>>>> }
>>>>>
>>>>> With 'uri' and 'channels' being mutually exclusive here.
>>>>>
>>>>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
>>>>> API wrt multifd - essentally the same data is now being set in two
>>>>> different places. IMHO, we should declare the 'multifd' capability
>>>>> and the 'multifd-chanels' parameter deprecated, since the information
>>>>> they provide is totally redundant, if you're giving an explicit list
>>>>> of channels to 'migrate'.
>>>> Hi Daniel. Initially while brainstorming this idea for the first time, we also came up with the same thought of depricating the migrate
>>>> API. But how will we achieve this now and how is it going to work. Is it like we will be making migate V2 APIs initially, integrate it and then
>>>> depricate the old one? would be happy to get some pointers from your end.
>>> Migration maintainers, please advise.
>> Hi Daniel and other migraton maintainers: Dr. David and Juan, what is your opinion on this. And how can we go forward implementing this. Some pointers and ideas from your end would be helpful too.
>> Regards,
>> Het Gala
>
> Just a gentle reminder Dr. David and Juan. Daniel already expressed
> his opinion to refactor the QAPI design for multifd. Your inputs and
> advice will also be very valuable to us. Thankyou.
>
> Regards,
> Het Gala
Hi all, I think the maintainers were occupied for quite sometime by KVM
forum work. This is again a gentle reminder mail for the migration
maintainers - Dr. David and Juan to share some ideas and inputs on
refactorisation of QAPI design for multifd suggested by Daniel, and how
this idea will pan out in the migration workflow. Looking forward for a
positive discussion on this topic :)
Regards,
Het Gala
[-- Attachment #2: Type: text/html, Size: 9747 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-08-02 7:53 ` Markus Armbruster
2022-08-08 6:11 ` Het Gala
@ 2022-11-21 12:26 ` Juan Quintela
2022-11-22 9:26 ` Daniel P. Berrangé
1 sibling, 1 reply; 22+ messages in thread
From: Juan Quintela @ 2022-11-21 12:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: Het Gala, Daniel P. Berrangé,
qemu-devel, dgilbert, pbonzini, eblake, prerna.saxena,
Manish Mishra
Markus Armbruster <armbru@redhat.com> wrote:
> Het Gala <het.gala@nutanix.com> writes:
Hi
>>>> # 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} ] } }
Why would one put the source uri and destination uri on the command?
It looks more complicated to me, but I guess there is a good reason.
>>>
>>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
>>> API wrt multifd - essentally the same data is now being set in two
>>> different places. IMHO, we should declare the 'multifd' capability
>>> and the 'multifd-chanels' parameter deprecated, since the information
>>> they provide is totally redundant, if you're giving an explicit list
>>> of channels to 'migrate'.
>>
>> Hi Daniel. Initially while brainstorming this idea for the first
>> time, we also came up with the same thought of depricating the
>> migrate
>> API. But how will we achieve this now and how is it going to
>> work. Is it like we will be making migate V2 APIs initially,
>> integrate it and then
>> depricate the old one? would be happy to get some pointers from your end.
>
> Migration maintainers, please advise.
I would put the old one in top of the new one, and call it a day.
I really hate the old one, but I haven't had the time to think about a
better one (nor I have had the time to look into this one).
The problem that I am seing here is that we are adding the number of
multifd channels here, and we were trying to not add migration
parameters into the migrate command.
BTW, once that we are at it, I guess we can just drop the inc/blk
parameters, we have had them deprecated ... forever?
Later, Juan.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair
2022-11-21 12:26 ` Juan Quintela
@ 2022-11-22 9:26 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-11-22 9:26 UTC (permalink / raw)
To: Juan Quintela
Cc: Markus Armbruster, Het Gala, qemu-devel, dgilbert, pbonzini,
eblake, prerna.saxena, Manish Mishra
On Mon, Nov 21, 2022 at 01:26:55PM +0100, Juan Quintela wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
> > Het Gala <het.gala@nutanix.com> writes:
>
>
>
> Hi
>
> >>>> # 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} ] } }
>
> Why would one put the source uri and destination uri on the command?
> It looks more complicated to me, but I guess there is a good reason.
>
> >>>
> >>> This whole scheme brings in redundancy wrt to the 'migrate-set-parameters'
> >>> API wrt multifd - essentally the same data is now being set in two
> >>> different places. IMHO, we should declare the 'multifd' capability
> >>> and the 'multifd-chanels' parameter deprecated, since the information
> >>> they provide is totally redundant, if you're giving an explicit list
> >>> of channels to 'migrate'.
> >>
> >> Hi Daniel. Initially while brainstorming this idea for the first
> >> time, we also came up with the same thought of depricating the
> >> migrate
> >> API. But how will we achieve this now and how is it going to
> >> work. Is it like we will be making migate V2 APIs initially,
> >> integrate it and then
> >> depricate the old one? would be happy to get some pointers from your end.
> >
> > Migration maintainers, please advise.
>
> I would put the old one in top of the new one, and call it a day.
> I really hate the old one, but I haven't had the time to think about a
> better one (nor I have had the time to look into this one).
>
> The problem that I am seing here is that we are adding the number of
> multifd channels here, and we were trying to not add migration
> parameters into the migrate command.
The issue of migration parameters is a much bigger one - a lot of them
should never have existed, if QEMU had a proper migration wire protocol
that could do feature negotiation.
We need to replace the wire protocol as the priority, at which point
the QMP side becomes simpler as a result. Starting with the QMP side,
without addressing the wire protocol first will never give us a good
long term result.
I've written more about that in my reply to Het's other patch.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-11-22 9:27 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 19:56 [PATCH v2 0/7] multifd: Multiple interface support on top of Multifd Het Gala
2022-07-21 19:56 ` [PATCH v2 1/7] multifd: adding more helper functions in util files for live migration Het Gala
2022-07-21 19:56 ` [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair Het Gala
2022-07-26 11:13 ` Daniel P. Berrangé
2022-07-28 15:02 ` Het Gala
2022-08-02 7:53 ` Markus Armbruster
2022-08-08 6:11 ` Het Gala
2022-08-08 9:29 ` Daniel P. Berrangé
2022-08-29 4:34 ` Het Gala
2022-09-21 10:08 ` Het Gala
2022-11-21 12:26 ` Juan Quintela
2022-11-22 9:26 ` Daniel P. Berrangé
2022-08-08 9:29 ` Daniel P. Berrangé
2022-07-21 19:56 ` [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side Het Gala
2022-07-26 11:20 ` Daniel P. Berrangé
2022-07-28 15:05 ` Het Gala
2022-07-21 19:56 ` [PATCH v2 4/7] multifd: HMP changes for multifd source and " Het Gala
2022-07-21 19:56 ` [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair Het Gala
2022-07-26 10:44 ` Daniel P. Berrangé
2022-07-28 15:15 ` Het Gala
2022-07-21 19:56 ` [PATCH v2 6/7] muitlfd: Correcting nit : whitespace error changes in qemu-sockets.c file Het Gala
2022-07-21 19:56 ` [PATCH v2 7/7] multifd: adding support for multifd connections dynamically Het Gala
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.