|
| From: | Het Gala |
| Subject: | Re: [PATCH 1/4] Modifying ‘migrate’ qmp command to add multi-FD socket on particular source and destination pair |
| Date: | Tue, 19 Jul 2022 13:21:34 +0530 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 |
On 19/07/22 12:36 pm, Markus Armbruster wrote:
> Okay, I understand your concern. I am okay with your approach too, but these things are not expected to change out of qmp command context. So is keeping @total_multifd_channel variable should be fine? or making a function is better?Het Gala <het.gala@nutanix.com> writes:On 18/07/22 8:03 pm, Markus Armbruster wrote:Het Gala <het.gala@nutanix.com> writes:On 18/07/22 2:05 pm, Markus Armbruster wrote:Het Gala <het.gala@nutanix.com> writes:i) Modified the format of the qemu monitor command : 'migrate' by adding a list, each element in the list consists of multi-FD connection parameters: source and destination uris and of the number of multi-fd channels between each pair. ii) Information of all multi-FD connection parameters’ list, length of the list and total number of multi-fd channels for all the connections together is stored in ‘OutgoingArgs’ struct. Suggested-by: Manish Mishra <manish.mishra@nutanix.com> Signed-off-by: Het Gala <het.gala@nutanix.com> ---[...]diff --git a/migration/socket.c b/migration/socket.c index 4fd5e85f50..7ca6af8cca 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -32,6 +32,17 @@ struct SocketOutgoingArgs { SocketAddress *saddr; } outgoing_args; +struct SocketArgs { + struct SrcDestAddr data; + uint8_t multifd_channels; +}; + +struct OutgoingMigrateParams { + struct SocketArgs *socket_args; + size_t length;Length of what?length of the socket_args[] array. Thanks for pointing it out. I will be more specific for this variable in the v2 patchset series.+ uint64_t total_multifd_channel;@total_multifd_channels appears to be the sum of the socket_args[*].multifd_channels. Correct?Yes Markus, you are correct.Sure you need to keep the sum separately?So earlier, the idea behind this was that, we had this intention to depreciate the migrate_multifd_channels() API from the live migration process. We made total_multifd_channels() function, where it used to calculate total number of multifd channels every time, for whichever function called was computation internsive so we replaced it by returning sum of socket_args[*].multifd_channels i.e. total_multifd_channel in the later patches. But now in the v2 patchset onwards, Thanks to inputs from Dr. David and Daniel, we are not depricating migrate_multifd_channels() API but the value from the API will be cross-referenced with sum of socket_args[*].multifd_channels i.e. total_multifd_channel, and error if they are not equal.I'm afraid I don't understand. I'm not sure I have to. Let me loop back to my question. If @total_multifd_channel is always the sum of the socket_args[*].multifd_channels, then you can always compute it on the fly. I.e. you can replace @total_multifd_channel by a function that returns the sum. Precomputing it instead is more complex, because then you need to document that the two are the same. Also, bug oppertunity: letting them deviate somehow. I figure that's worthwhile only if computing on the fly is too expensive.
+} outgoing_migrate_params; + void socket_send_channel_create(QIOTaskFunc f, void *data) { QIOChannelSocket *sioc = qio_channel_socket_new(); @@ -47,6 +58,14 @@ int socket_send_channel_destroy(QIOChannel *send) qapi_free_SocketAddress(outgoing_args.saddr); outgoing_args.saddr = NULL; } + + if (outgoing_migrate_params.socket_args != NULL) { + g_free(outgoing_migrate_params.socket_args); + outgoing_migrate_params.socket_args = NULL; + } + if (outgoing_migrate_params.length) { + outgoing_migrate_params.length = 0; + } return 0; } @@ -117,13 +136,41 @@ socket_start_outgoing_migration_internal(MigrationState *s, } void socket_start_outgoing_migration(MigrationState *s, - const char *str, + const char *dst_str, Error **errp) { Error *err = NULL; - SocketAddress *saddr = socket_parse(str, &err); + SocketAddress *dst_saddr = socket_parse(dst_str, &err); + if (!err) { + socket_start_outgoing_migration_internal(s, dst_saddr, &err); + } + error_propagate(errp, err); +} + +void init_multifd_array(int length) +{ + outgoing_migrate_params.socket_args = g_new0(struct SocketArgs, length); + outgoing_migrate_params.length = length; + outgoing_migrate_params.total_multifd_channel = 0; +} + +void store_multifd_migration_params(const char *dst_uri, + const char *src_uri, + uint8_t multifd_channels, + int idx, Error **errp) +{ + Error *err = NULL; + SocketAddress *src_addr = NULL; + SocketAddress *dst_addr = socket_parse(dst_uri, &err); + if (src_uri) { + src_addr = socket_parse(src_uri, &err); + }Incorrect use of &err. error.h's big comment: * Receive and accumulate multiple errors (first one wins): * Error *err = NULL, *local_err = NULL; * foo(arg, &err); * bar(arg, &local_err); * error_propagate(&err, local_err); * if (err) { * handle the error... * } * * Do *not* "optimize" this to * Error *err = NULL; * foo(arg, &err); * bar(arg, &err); // WRONG! * if (err) { * handle the error... * } * because this may pass a non-null err to bar().Thankyou Markus for sharing this knowledge. I was unaware of the dont's with &err.The big comment should help you along. If it doesn't, just ask. I read the comment, and it is pretty well explained out there.if (!err) { - socket_start_outgoing_migration_internal(s, saddr, &err); + outgoing_migrate_params.socket_args[idx].data.dst_addr = dst_addr; + outgoing_migrate_params.socket_args[idx].data.src_addr = src_addr; + outgoing_migrate_params.socket_args[idx].multifd_channels + = multifd_channels; + outgoing_migrate_params.total_multifd_channel += multifd_channels; } error_propagate(errp, err);Consider struct SocketArgs *sa = &outgoing_migrate_params.socket_args[idx]; SocketAddress *src_addr, *dst_addr; src_addr = socketaddress(src_uri, errp); if (!src_addr) { return; } dst_addr = socketaddress(dst_uri, errp); if (!dst_addr) { return; } sa->data.dst_addr = dst_addr; sa->data.src_addr = src_addr; sa->multifd_channels = multifd_channels; outgoing_migrate_params.total_multifd_channel += multifd_channels;Thanks Markus for this amazing suggestion. Your approach looks simpler to understand and also resolves the error it had with &err. I will surely implement this in the upcoming v2 patchset.You're welcome :)I just wanted to have a double check on the solution you gave above Markus. The suggestion you have given there has been deliberately written in that way right, because src_addr = socketaddress(src_uri, errp); dst_addr = socketaddress(dst_uri, errp); if (!src_addr) { return; } if (!dst_addr) { return; } would still be an error right according to the &err guidelines from error.h file.Correct.
> Thankyou Markus.
}[...]diff --git a/qapi/migration.json b/qapi/migration.json index 6130cd9fae..fb259d626b 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1454,12 +1454,38 @@ ## { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } +## +# @MigrateUriParameter: +# +# Information regarding which source interface is connected to which +# destination interface and number of multifd channels over each interface. +# +# @source-uri: the Uniform Resource Identifier of the source VM. +# Default port number is 0. +# +# @destination-uri: the Uniform Resource Identifier of the destination VM +# +# @multifd-channels: number of parallel multifd channels used to migrate data +# for specific source-uri and destination-uri. Default value +# in this case is 2 (Since 4.0)You mean "(Since 7.1)", I guess.Yes yes. Also David pointed this thing out. I will update the version in the v2 patchset.+# +## +{ 'struct' : 'MigrateUriParameter', + 'data' : { 'source-uri' : 'str', + 'destination-uri' : 'str', + '*multifd-channels' : 'uint8'} } + ## # @migrate: # # Migrates the current running guest to another Virtual Machine. # # @uri: the Uniform Resource Identifier of the destination VM +# for migration thread +# +# @multi-fd-uri-list: list of pair of source and destination VM Uniform +# Resource Identifiers with number of multifd-channels +# for each pair # # @blk: do block migration (full disk copy) # @@ -1479,20 +1505,27 @@ # 1. The 'query-migrate' command should be used to check migration's progress # and final result (this information is provided by the 'status' member) # -# 2. All boolean arguments default to false +# 2. The uri argument should have the Uniform Resource Identifier of default +# destination VM. This connection will be bound to default network +# +# 3. All boolean arguments default to false # -# 3. The user Monitor's "detach" argument is invalid in QMP and should not +# 4. The user Monitor's "detach" argument is invalid in QMP and should not # be used # # Example: # -# -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } +# -> { "execute": "migrate", +# "arguments": { "uri": "tcp:0:4446", "multi-fd-uri-list": [ { +# "source-uri": "tcp::6900", "destination-uri": "tcp:0:4480", +# "multifd-channels": 4}, { "source-uri": "tcp:10.0.0.0: ", +# "destination-uri": "tcp:11.0.0.0:7789", "multifd-channels": 5} ] } } # <- { "return": {} } # ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', - '*detach': 'bool', '*resume': 'bool' } } + 'data': {'uri': 'str', '*multi-fd-uri-list': ['MigrateUriParameter'], '*blk': 'bool',?? Sorry Markus, I think the statement I wrote did not make sense, I apologise for that. I meant to say example in the sense: # Example: # # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } # -> { "execute": "migrate", # "arguments": { "uri": "tcp:0:4446", "multi-fd-uri-list": [ { # "source-uri": "tcp::6900", "destination-uri": "tcp:0:4480", # "multifd-channels": 4}, { "source-uri": "tcp:10.0.0.0: ", # "destination-uri": "tcp:11.0.0.0:7789", "multifd-channels": 5} ] } } even this we should try to wrap within 80 character column right? or is that okay to go beyond 80.I'd format it like # -> { "execute": "migrate", # "arguments": { # "uri": "tcp:0:4446", # "multi-fd-uri-list": [ # { "source-uri": "tcp::6900", # "destination-uri": "tcp:0:4480", # "multifd-channels": 4 }, # { "source-uri": "tcp:10.0.0.0: ", # "destination-uri": "tcp:11.0.0.0:7789", # "multifd-channels": 5} ] } }
> Yeah sure Markus.
Long line.Okay, acknowledged. Even for example, should it be under 80 characters per line, or that is fine?docs/devel/style.rst: Line width ========== Lines should be 80 characters; try not to make them longer. Sometimes it is hard to do, especially when dealing with QEMU subsystems that use long function or symbol names. If wrapping the line at 80 columns is obviously less readable and more awkward, prefer not to wrap it; better to have an 85 character line than one which is awkwardly wrapped. Even in that case, try not to make lines much longer than 80 characters. (The checkpatch script will warn at 100 characters, but this is intended as a guard against obviously-overlength lines, not a target.) Personally, I very much prefer to wrap between 70 and 75 except where it impairs legibility.Okay thanks again Markus for your valuable suggestion. I will try to wrap within 75 in almost all the cases.+ '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } ## # @migrate-incoming:Regards, Het GalaRegards, Het Gala
Regards, Het Gala
| [Prev in Thread] | Current Thread | [Next in Thread] |