Het Gala <het.g...@nutanix.com> writes:
> On 18/07/22 8:03 pm, Markus Armbruster wrote:
>> Het Gala <het.g...@nutanix.com> writes:
>>
>>> On 18/07/22 2:05 pm, Markus Armbruster wrote:
>>>> Het Gala <het.g...@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.mis...@nutanix.com>
>>>>> Signed-off-by: Het Gala <het.g...@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.
>>>>> }
[...]
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index 6130cd9fae..fb259d626b 100644
>>>>> --- a/qapi/migration.json
>>>>> +++ b/qapi/migration.json
>>>>> @@ -1454,12 +1454,38 @@
>>>>> ##
>>>>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>>>
>>>>> +##
>>>>> +# @MigrateUriParameter:
>>>>> +#
>>>>> +# Information regarding which source interface is connected to which
>>>>> +# destination interface and number of multifd channels over each
>>>>> interface.
>>>>> +#
>>>>> +# @source-uri: the Uniform Resource Identifier of the source VM.
>>>>> +# Default port number is 0.
>>>>> +#
>>>>> +# @destination-uri: the Uniform Resource Identifier of the destination VM
>>>>> +#
>>>>> +# @multifd-channels: number of parallel multifd channels used to migrate
>>>>> data
>>>>> +# for specific source-uri and destination-uri.
>>>>> Default value
>>>>> +# in this case is 2 (Since 4.0)
>>>> You mean "(Since 7.1)", I guess.
>>> Yes yes. Also David pointed this thing out. I will update the version
>>> in the v2 patchset.
>>>
>>>>> +#
>>>>> +##
>>>>> +{ 'struct' : 'MigrateUriParameter',
>>>>> + 'data' : { 'source-uri' : 'str',
>>>>> + 'destination-uri' : 'str',
>>>>> + '*multifd-channels' : 'uint8'} }
>>>>> +
>>>>> ##
>>>>> # @migrate:
>>>>> #
>>>>> # Migrates the current running guest to another Virtual Machine.
>>>>> #
>>>>> # @uri: the Uniform Resource Identifier of the destination VM
>>>>> +# for migration thread
>>>>> +#
>>>>> +# @multi-fd-uri-list: list of pair of source and destination VM Uniform
>>>>> +# Resource Identifiers with number of
>>>>> multifd-channels
>>>>> +# for each pair
>>>>> #
>>>>> # @blk: do block migration (full disk copy)
>>>>> #
>>>>> @@ -1479,20 +1505,27 @@
>>>>> # 1. The 'query-migrate' command should be used to check migration's
>>>>> progress
>>>>> # and final result (this information is provided by the 'status'
>>>>> member)
>>>>> #
>>>>> -# 2. All boolean arguments default to false
>>>>> +# 2. The uri argument should have the Uniform Resource Identifier of
>>>>> default
>>>>> +# destination VM. This connection will be bound to default network
>>>>> +#
>>>>> +# 3. All boolean arguments default to false
>>>>> #
>>>>> -# 3. The user Monitor's "detach" argument is invalid in QMP and should
>>>>> not
>>>>> +# 4. The user Monitor's "detach" argument is invalid in QMP and should
>>>>> not
>>>>> # be used
>>>>> #
>>>>> # Example:
>>>>> #
>>>>> -# -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>>>> +# -> { "execute": "migrate",
>>>>> +# "arguments": { "uri": "tcp:0:4446",
>>>>> "multi-fd-uri-list": [ {
>>>>> +# "source-uri": "tcp::6900",
>>>>> "destination-uri": "tcp:0:4480",
>>>>> +# "multifd-channels": 4}, { "source-uri":
>>>>> "tcp:10.0.0.0: ",
>>>>> +# "destination-uri": "tcp:11.0.0.0:7789",
>>>>> "multifd-channels": 5} ] } }
>>>>> # <- { "return": {} }
>>>>> #
>>>>> ##
>>>>> { 'command': 'migrate',
>>>>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>>>>> - '*detach': 'bool', '*resume': 'bool' } }
>>>>> + 'data': {'uri': 'str', '*multi-fd-uri-list': ['MigrateUriParameter'],
>>>>> '*blk': 'bool',
> ??
>
> Sorry Markus, I think the statement I wrote did not make sense, I apologise
> for that. I meant to say example in the sense:
>
> # Example:
> #
> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> # -> { "execute": "migrate",
> # "arguments": { "uri": "tcp:0:4446", "multi-fd-uri-list": [ {
> # "source-uri": "tcp::6900",
> "destination-uri": "tcp:0:4480",
> # "multifd-channels": 4}, { "source-uri":
> "tcp:10.0.0.0: ",
> # "destination-uri": "tcp:11.0.0.0:7789",
> "multifd-channels": 5} ] } }
>
> even this we should try to wrap within 80 character column right? or is that
> okay to go beyond 80.
I'd format it like
# -> { "execute": "migrate",
# "arguments": {
# "uri": "tcp:0:4446",
# "multi-fd-uri-list": [
# { "source-uri": "tcp::6900",
# "destination-uri": "tcp:0:4480",
# "multifd-channels": 4 },
# { "source-uri": "tcp:10.0.0.0: ",
# "destination-uri": "tcp:11.0.0.0:7789",
# "multifd-channels": 5} ] } }
>>>> Long line.
>>> Okay, acknowledged. Even for example, should it be under 80
>>> characters per line, or that is fine?
>> docs/devel/style.rst:
>>
>> Line width
>> ==========
>>
>> Lines should be 80 characters; try not to make them longer.
>>
>> Sometimes it is hard to do, especially when dealing with QEMU subsystems
>> that use long function or symbol names. If wrapping the line at 80
>> columns
>> is obviously less readable and more awkward, prefer not to wrap it;
>> better
>> to have an 85 character line than one which is awkwardly wrapped.
>>
>> Even in that case, try not to make lines much longer than 80 characters.
>> (The checkpatch script will warn at 100 characters, but this is intended
>> as a guard against obviously-overlength lines, not a target.)
>>
>> Personally, I very much prefer to wrap between 70 and 75 except where it
>> impairs legibility.
> Okay thanks again Markus for your valuable suggestion. I will try to wrap
> within 75 in almost all the cases.
>>>>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>>> ##
>>>>> # @migrate-incoming:
>>> Regards,
>>>
>>> Het Gala
>
> Regards,
>
> Het Gala
