qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Maciej S. Szmigiero
Subject: Re: [PATCH v2 09/17] migration/multifd: Device state transfer support - receive side
Date: Thu, 19 Sep 2024 21:59:01 +0200
User-agent: Mozilla Thunderbird 
On 12.09.2024 15:52, Fabiano Rosas wrote:

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


That's because MultiFDPacket_t::ramblock is missing QEMU_NONSTRING
annotation (like the proposed MultiFDPacketDeviceState_t::idstr has)
so Coverity assumes it was meant to be a NULL-terminated C-string.


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


Thanks,
Maciej
reply via email to
[Prev in Thread] Current Thread [Next in Thread]