Re: [PATCH v4 13/33] migration/multifd: Device state transfer support - receive side
On 3.02.2025 23:59, Peter Xu wrote: On Mon, Feb 03, 2025 at 11:18:11PM +0100, Maciej S. Szmigiero wrote: On 3.02.2025 22:27, Peter Xu wrote: On Thu, Jan 30, 2025 at 11:08:34AM +0100, Maciej S. Szmigiero wrote: From: "Maciej S. Szmigiero" Add a basic support for receiving device state via multifd channels - channels that are shared with RAM transfers. 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 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 I think I acked this one. You could keep my R-b if... [...] diff --git a/migration/multifd.h b/migration/multifd.h index 9e4baa066312..abf3acdcee40 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -62,6 +62,12 @@ MultiFDRecvData *multifd_get_recv_data(void); #define MULTIFD_FLAG_UADK (8 << 1) #define MULTIFD_FLAG_QATZIP (16 << 1) +/* + * If set it means that this packet contains device state + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t). + */ +#define MULTIFD_FLAG_DEVICE_STATE (1 << 6) ... if this won't conflict with MULTIFD_FLAG_QATZIP. Hmm, isn't (16 << 1) = 32 while (1 << 6) = 64? Oops. :) I think we should stick with one way to write it, then when rebase you can see such conflicts - either your patch uses 32 << 1, or perhaps we should start to switch to BIT() for all above instead.. Still, do you mind switch to "32 << 1" (or use BIT())? I will switch to 32 << 1 for consistency with the compression flags above. > With either, feel free to take: Reviewed-by: Peter Xu Thanks, Maciej
Re: [PATCH v4 13/33] migration/multifd: Device state transfer support - receive side
On 3.02.2025 22:27, Peter Xu wrote: On Thu, Jan 30, 2025 at 11:08:34AM +0100, Maciej S. Szmigiero wrote: From: "Maciej S. Szmigiero" Add a basic support for receiving device state via multifd channels - channels that are shared with RAM transfers. 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 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 I think I acked this one. You could keep my R-b if... [...] diff --git a/migration/multifd.h b/migration/multifd.h index 9e4baa066312..abf3acdcee40 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -62,6 +62,12 @@ MultiFDRecvData *multifd_get_recv_data(void); #define MULTIFD_FLAG_UADK (8 << 1) #define MULTIFD_FLAG_QATZIP (16 << 1) +/* + * If set it means that this packet contains device state + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t). + */ +#define MULTIFD_FLAG_DEVICE_STATE (1 << 6) ... if this won't conflict with MULTIFD_FLAG_QATZIP. Hmm, isn't (16 << 1) = 32 while (1 << 6) = 64? I think we should stick with one way to write it, then when rebase you can see such conflicts - either your patch uses 32 << 1, or perhaps we should start to switch to BIT() for all above instead.. Thanks, Maciej
Re: [PATCH v4 13/33] migration/multifd: Device state transfer support - receive side
On Mon, Feb 03, 2025 at 11:18:11PM +0100, Maciej S. Szmigiero wrote: > On 3.02.2025 22:27, Peter Xu wrote: > > On Thu, Jan 30, 2025 at 11:08:34AM +0100, Maciej S. Szmigiero wrote: > > > From: "Maciej S. Szmigiero" > > > > > > Add a basic support for receiving device state via multifd channels - > > > channels that are shared with RAM transfers. > > > > > > 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 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 > > > > I think I acked this one. You could keep my R-b if... > > > > [...] > > > > > diff --git a/migration/multifd.h b/migration/multifd.h > > > index 9e4baa066312..abf3acdcee40 100644 > > > --- a/migration/multifd.h > > > +++ b/migration/multifd.h > > > @@ -62,6 +62,12 @@ MultiFDRecvData *multifd_get_recv_data(void); > > > #define MULTIFD_FLAG_UADK (8 << 1) > > > #define MULTIFD_FLAG_QATZIP (16 << 1) > > > +/* > > > + * If set it means that this packet contains device state > > > + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t). > > > + */ > > > +#define MULTIFD_FLAG_DEVICE_STATE (1 << 6) > > > > ... if this won't conflict with MULTIFD_FLAG_QATZIP. > > Hmm, isn't (16 << 1) = 32 while (1 << 6) = 64? Oops. :) > > I think we should stick with one way to write it, then when rebase you can > > see such conflicts - either your patch uses 32 << 1, or perhaps we should > > start to switch to BIT() for all above instead.. Still, do you mind switch to "32 << 1" (or use BIT())? With either, feel free to take: Reviewed-by: Peter Xu -- Peter Xu
Re: [PATCH v4 13/33] migration/multifd: Device state transfer support - receive side
On Thu, Jan 30, 2025 at 11:08:34AM +0100, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" > > Add a basic support for receiving device state via multifd channels - > channels that are shared with RAM transfers. > > 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 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 I think I acked this one. You could keep my R-b if... [...] > diff --git a/migration/multifd.h b/migration/multifd.h > index 9e4baa066312..abf3acdcee40 100644 > --- a/migration/multifd.h > +++ b/migration/multifd.h > @@ -62,6 +62,12 @@ MultiFDRecvData *multifd_get_recv_data(void); > #define MULTIFD_FLAG_UADK (8 << 1) > #define MULTIFD_FLAG_QATZIP (16 << 1) > > +/* > + * If set it means that this packet contains device state > + * (MultiFDPacketDeviceState_t), not RAM data (MultiFDPacket_t). > + */ > +#define MULTIFD_FLAG_DEVICE_STATE (1 << 6) ... if this won't conflict with MULTIFD_FLAG_QATZIP. I think we should stick with one way to write it, then when rebase you can see such conflicts - either your patch uses 32 << 1, or perhaps we should start to switch to BIT() for all above instead.. -- Peter Xu
[PATCH v4 13/33] migration/multifd: Device state transfer support - receive side
From: "Maciej S. Szmigiero"
Add a basic support for receiving device state via multifd channels -
channels that are shared with RAM transfers.
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 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
---
migration/multifd.c | 99 -
migration/multifd.h | 26 +++-
2 files changed, 113 insertions(+), 12 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 53493676012e..810e7b1fb340 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"
@@ -252,14 +253,24 @@ static int
multifd_recv_unfill_packet_header(MultiFDRecvParams *p,
return 0;
}
-static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
+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)
{
const 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++;
/* Always unfill, old QEMUs (<9.0) send data along with SYNC */
ret = multifd_ram_unfill_packet(p, errp);
@@ -270,6 +281,17 @@ 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);
+}
+
+return multifd_recv_unfill_packet_ram(p, errp);
+}
+
static bool multifd_send_should_exit(void)
{
return qatomic_read(&multifd_send_state->exiting);
@@ -1027,6 +1049,7 @@ static void
multifd_recv_cleanup_channel(MultiFDRecvParams *p)
p->packet_len = 0;
g_free(p->packet);
p->packet = NULL;
+g_clear_pointer(&p->packet_dev_state, g_free);
g_free(p->normal);
p->normal = NULL;
g_free(p->zero);
@@ -1128,6 +1151,32 @@ void multifd_recv_sync_main(void)
trace_multifd_recv_sync_main(multifd_recv_state->packet_num);
}
+static int multifd_device_state_recv(MultiFDRecvParams *p, Error **errp)
+{
+g_autofree char *idstr = NULL;
+g_autofree char *dev_state_buf = NULL;
+int ret;
+
+dev_state_buf = g_malloc(p->next_packet_size);
+
+ret = qio_channel_read_all(p->c, dev_state_buf, p->next_packet_size, errp);
+if (ret != 0) {
+return ret;
+}
+
+idstr = g_strndup(p->packet_dev_state->idstr,
+ sizeof(p->packet_dev_state->idstr));
+
+if (!qemu_loadvm_load_state_buffer(idstr,
+ p->packet_dev_state->instance_id,
+ dev_state_buf, p->next_packet_size,
+ errp)) {
+ret = -1;
+}
+
+return ret;
+}
+
static void *multifd_recv_thread(void *opaque)
{
MultiFDRecvParams *p = opaque;
@@ -1141,6 +1190,7 @@ static void *multifd_recv_thread(void *opaque)
while (true) {
MultiFDPacketHdr_t hdr;
uint32_t flags = 0;
+bool is_device_state = false;
bool has_data = false;
uint8_t *pkt_buf;
size_t pkt_len;
@@ -1163,8 +1213,14 @@ static void *multifd_recv_thread(void *opaque)
break;
}
-pkt_buf = (uint8_t *)p->packet + sizeof(hdr);
-pkt_len = p->packet_len - sizeof(hdr);
+is_device_state = p->flags & MULTIFD_FLAG_DEVICE_STATE;
+if (is_device_state) {
+pkt_buf = (uint8_t *)p->packet_dev_state + sizeof(hdr);
+pkt_len = sizeof(*p->packet_dev_state) - sizeof(hdr);
+} else {
+pkt_buf = (uint8_t *)p->packet + sizeof(hdr);
+pkt_len = p->packet_len - sizeof(hdr);
+}
ret = qio_channel_read_all_eof(p->c, (char *)pkt_buf, pkt_len,
&local_err);
@@ -1183,12 +1239,17 @@ static void *multifd_recv_thread(void *opaque)
/* recv methods don't know how to handle the SYNC flag */
p->flags &= ~MULTIFD_FLAG_SYNC;
-/*
- * Even if it
