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

[Markus Armbruster <armbru@redhat.com>]

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.

>>>>>    }

[...]

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