Re: [PATCH v2 09/17] migration/multifd: Device state transfer support - receive side

[Fabiano Rosas <farosas@suse.de>]

Avihai Horon <avihaih@nvidia.com> writes:

> On 09/09/2024 21:05, Maciej S. Szmigiero wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5.09.2024 18:47, Avihai Horon wrote:
>>>
>>> On 27/08/2024 20:54, Maciej S. Szmigiero wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>
>>>> Add a basic support for receiving device state via multifd channels -
>>>> channels that are shared with RAM transfers.
>>>>
>>>> To differentiate between a device state and a RAM packet the packet
>>>> header is read first.
>>>>
>>>> Depending whether MULTIFD_FLAG_DEVICE_STATE flag is present or not 
>>>> in the
>>>> packet header either device state (MultiFDPacketDeviceState_t) or RAM
>>>> data (existing MultiFDPacket_t) is then read.
>>>>
>>>> The received device state data is provided to
>>>> qemu_loadvm_load_state_buffer() function for processing in the
>>>> device's load_state_buffer handler.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>> ---
>>>>   migration/multifd.c | 127 
>>>> +++++++++++++++++++++++++++++++++++++-------
>>>>   migration/multifd.h |  31 ++++++++++-
>>>>   2 files changed, 138 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>>> index b06a9fab500e..d5a8e5a9c9b5 100644
>>>> --- a/migration/multifd.c
>>>> +++ b/migration/multifd.c
>>>> @@ -21,6 +21,7 @@
>>>>   #include "file.h"
>>>>   #include "migration.h"
>>>>   #include "migration-stats.h"
>>>> +#include "savevm.h"
>>>>   #include "socket.h"
>>>>   #include "tls.h"
>>>>   #include "qemu-file.h"
>>>> @@ -209,10 +210,10 @@ void 
>>>> multifd_send_fill_packet(MultiFDSendParams *p)
>>>>
>>>>       memset(packet, 0, p->packet_len);
>>>>
>>>> -    packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>>>> -    packet->version = cpu_to_be32(MULTIFD_VERSION);
>>>> +    packet->hdr.magic = cpu_to_be32(MULTIFD_MAGIC);
>>>> +    packet->hdr.version = cpu_to_be32(MULTIFD_VERSION);
>>>>
>>>> -    packet->flags = cpu_to_be32(p->flags);
>>>> +    packet->hdr.flags = cpu_to_be32(p->flags);
>>>>       packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>>>>
>>>>       packet_num = qatomic_fetch_inc(&multifd_send_state->packet_num);
>>>> @@ -228,31 +229,49 @@ void 
>>>> multifd_send_fill_packet(MultiFDSendParams *p)
>>>>                               p->flags, p->next_packet_size);
>>>>   }
>>>>
>>>> -static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error 
>>>> **errp)
>>>> +static int multifd_recv_unfill_packet_header(MultiFDRecvParams *p,
>>>> + MultiFDPacketHdr_t *hdr,
>>>> +                                             Error **errp)
>>>>   {
>>>> -    MultiFDPacket_t *packet = p->packet;
>>>> -    int ret = 0;
>>>> -
>>>> -    packet->magic = be32_to_cpu(packet->magic);
>>>> -    if (packet->magic != MULTIFD_MAGIC) {
>>>> +    hdr->magic = be32_to_cpu(hdr->magic);
>>>> +    if (hdr->magic != MULTIFD_MAGIC) {
>>>>           error_setg(errp, "multifd: received packet "
>>>>                      "magic %x and expected magic %x",
>>>> -                   packet->magic, MULTIFD_MAGIC);
>>>> +                   hdr->magic, MULTIFD_MAGIC);
>>>>           return -1;
>>>>       }
>>>>
>>>> -    packet->version = be32_to_cpu(packet->version);
>>>> -    if (packet->version != MULTIFD_VERSION) {
>>>> +    hdr->version = be32_to_cpu(hdr->version);
>>>> +    if (hdr->version != MULTIFD_VERSION) {
>>>>           error_setg(errp, "multifd: received packet "
>>>>                      "version %u and expected version %u",
>>>> -                   packet->version, MULTIFD_VERSION);
>>>> +                   hdr->version, MULTIFD_VERSION);
>>>>           return -1;
>>>>       }
>>>>
>>>> -    p->flags = be32_to_cpu(packet->flags);
>>>> +    p->flags = be32_to_cpu(hdr->flags);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int 
>>>> multifd_recv_unfill_packet_device_state(MultiFDRecvParams *p,
>>>> +                                                   Error **errp)
>>>> +{
>>>> +    MultiFDPacketDeviceState_t *packet = p->packet_dev_state;
>>>> +
>>>> +    packet->instance_id = be32_to_cpu(packet->instance_id);
>>>> +    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int multifd_recv_unfill_packet_ram(MultiFDRecvParams *p, 
>>>> Error **errp)
>>>> +{
>>>> +    MultiFDPacket_t *packet = p->packet;
>>>> +    int ret = 0;
>>>> +
>>>>       p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>>>>       p->packet_num = be64_to_cpu(packet->packet_num);
>>>> -    p->packets_recved++;
>>>>
>>>>       if (!(p->flags & MULTIFD_FLAG_SYNC)) {
>>>>           ret = multifd_ram_unfill_packet(p, errp);
>>>> @@ -264,6 +283,19 @@ static int 
>>>> multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>>>       return ret;
>>>>   }
>>>>
>>>> +static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error 
>>>> **errp)
>>>> +{
>>>> +    p->packets_recved++;
>>>> +
>>>> +    if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
>>>> +        return multifd_recv_unfill_packet_device_state(p, errp);
>>>> +    } else {
>>>> +        return multifd_recv_unfill_packet_ram(p, errp);
>>>> +    }
>>>> +
>>>> +    g_assert_not_reached();
>>>
>>> We can drop the assert and the "else":
>>> if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
>>>      return multifd_recv_unfill_packet_device_state(p, errp);
>>> }
>>>
>>> return multifd_recv_unfill_packet_ram(p, errp);
>>
>> Ack.
>>
>>>> +}
>>>> +
>>>>   static bool multifd_send_should_exit(void)
>>>>   {
>>>>       return qatomic_read(&multifd_send_state->exiting);
>>>> diff --git a/migration/multifd.h b/migration/multifd.h
>>>> index a3e35196d179..a8f3e4838c01 100644
>>>> --- a/migration/multifd.h
>>>> +++ b/migration/multifd.h
>>>> @@ -45,6 +45,12 @@ MultiFDRecvData *multifd_get_recv_data(void);
>>>>   #define MULTIFD_FLAG_QPL (4 << 1)
>>>>   #define MULTIFD_FLAG_UADK (8 << 1)
>>>>
>>>> +/*
>>>> + * If set it means that this packet contains device state
>>>> + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t).
>>>> + */
>>>> +#define MULTIFD_FLAG_DEVICE_STATE (1 << 4)
>>>> +
>>>>   /* This value needs to be a multiple of qemu_target_page_size() */
>>>>   #define MULTIFD_PACKET_SIZE (512 * 1024)
>>>>
>>>> @@ -52,6 +58,11 @@ typedef struct {
>>>>       uint32_t magic;
>>>>       uint32_t version;
>>>>       uint32_t flags;
>>>> +} __attribute__((packed)) MultiFDPacketHdr_t;
>>>
>>> Maybe split this patch into two: one that adds the packet header 
>>> concept and another that adds the new device packet?
>>
>> Can do.
>>
>>>> +
>>>> +typedef struct {
>>>> +    MultiFDPacketHdr_t hdr;
>>>> +
>>>>       /* maximum number of allocated pages */
>>>>       uint32_t pages_alloc;
>>>>       /* non zero pages */
>>>> @@ -72,6 +83,16 @@ typedef struct {
>>>>       uint64_t offset[];
>>>>   } __attribute__((packed)) MultiFDPacket_t;
>>>>
>>>> +typedef struct {
>>>> +    MultiFDPacketHdr_t hdr;
>>>> +
>>>> +    char idstr[256] QEMU_NONSTRING;
>>>
>>> idstr should be null terminated, or am I missing something?
>>
>> There's no need to always NULL-terminate a constant-size field,
>> since the strncpy() already stops at the field size, so we can
>> gain another byte for actual string use this way.
>>
>> RAM block idstr also uses the same "trick":
>>> void multifd_ram_fill_packet(MultiFDSendParams *p):
>>> strncpy(packet->ramblock, pages->block->idstr, 256);
>>
> But can idstr actually be 256 bytes long without null byte?
> There are a lot of places where idstr is a parameter for functions that 
> expect null terminated string and it is also printed as such.

Yeah, and I actually don't see the "trick" being used in
RAMBlock. Anyway, it's best to null terminate to be more predictable. We
also had Coverity reports about similar things:

https://lore.kernel.org/r/CAFEAcA_F2qrSAacY=V5Hez1qFGuNW0-XqL2LQ=Y_UKYuHEJWhw@mail.gmail.com

I haven't got the time to send that patch yet.

>
> Thanks.