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

[Markus Armbruster]

Het Gala <het.gala@nutanix.com> writes:

> On 19/07/22 12:36 pm, Markus Armbruster wrote:
>> 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.
>> Okay, I understand your concern. I am okay with your approach too, but these 
>> things are not expected to change out of qmp command context. 
>
> So is keeping @total_multifd_channel variable should be fine? or making a 
> function is better?

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

[...]