[PATCH 0/6] More migration improvements

[PATCH 0/6] More migration improvements

[David Gibson]

Here's another batch of migration changes.  The first 5 patches are
fixups for fairly minor issues in the existing draft patches.  It
probably makes sense to fold them into the existing patches.

The last one changes the structure of the migration callbacks /
sections with a new scheme that's simpler and more flexible.
Specifically it will allow callbacks which "stream" data in a format
designed for migration rather than being identical to the in memory
structure.

David Gibson (6):
  vhost-user: Change different vhost-user messages to trace() level
  migrate, flow: Abort migration on repair_flush() failure
  migrate: Clearer debug message in migrate_request()
  migrate: Handle sending header section from data sections
  util: read_remainder should take const pointer to iovec
  migrate: Make migration handlers simpler and more flexible

 flow.c       |  17 +++-
 flow.h       |   6 +-
 migrate.c    | 277 ++++++++++++++++++---------------------------------
 migrate.h    |  54 ++++++----
 util.c       |   2 +-
 util.h       |   2 +-
 vhost_user.c |   8 +-
 vu_common.c  |   6 +-
 8 files changed, 155 insertions(+), 217 deletions(-)

-- 
2.48.1

[PATCH 1/6] vhost-user: Change different vhost-user messages to trace() level

[David Gibson]

A previous patch changed the debug() calls in vu_control_handler() to
trace() to reduce noise when debugging migration.  However those messages
are infrequent, and pretty useful when debugging migration (I used them to
discover I needed a different qemu version).

At the same time we have potentially per-packet debug() messages in
vu_kick_cb() and vu_send_single().  These get much more in the way since
they occur asynchronously with the migration operation.  As a rule, per
packet messages should be trace() level anyway.

[This is a candidate to fold into the earlier patch]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 vhost_user.c | 8 ++++----
 vu_common.c  | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/vhost_user.c b/vhost_user.c
index 19ede8a9..755017f1 100644
--- a/vhost_user.c
+++ b/vhost_user.c
@@ -1181,11 +1181,11 @@ void vu_control_handler(struct vu_dev *vdev, int fd, uint32_t events)
 		vu_sock_reset(vdev);
 		return;
 	}
-	trace("================ Vhost user message ================");
-	trace("Request: %s (%d)", vu_request_to_string(msg.hdr.request),
+	debug("================ Vhost user message ================");
+	debug("Request: %s (%d)", vu_request_to_string(msg.hdr.request),
 		msg.hdr.request);
-	trace("Flags:   0x%x", msg.hdr.flags);
-	trace("Size:    %u", msg.hdr.size);
+	debug("Flags:   0x%x", msg.hdr.flags);
+	debug("Size:    %u", msg.hdr.size);
 
 	need_reply = msg.hdr.flags & VHOST_USER_NEED_REPLY_MASK;
 
diff --git a/vu_common.c b/vu_common.c
index 78d1c1ba..48826b13 100644
--- a/vu_common.c
+++ b/vu_common.c
@@ -240,7 +240,7 @@ void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,
 	if (rc == -1)
 		die_perror("vhost-user kick eventfd_read()");
 
-	debug("vhost-user: got kick_data: %016"PRIx64" idx: %d",
+	trace("vhost-user: got kick_data: %016"PRIx64" idx: %d",
 	      kick_data, ref.queue);
 	if (VHOST_USER_IS_QUEUE_TX(ref.queue))
 		vu_handle_tx(vdev, ref.queue, now);
@@ -264,7 +264,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
 	int elem_cnt;
 	int i;
 
-	debug("vu_send_single size %zu", size);
+	trace("vu_send_single size %zu", size);
 
 	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
 		debug("Got packet, but RX virtqueue not usable yet");
@@ -296,7 +296,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
 
 	vu_flush(vdev, vq, elem, elem_cnt);
 
-	debug("vhost-user sent %zu", total);
+	trace("vhost-user sent %zu", total);
 
 	return total;
 err:
-- 
@@ -240,7 +240,7 @@ void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,
 	if (rc == -1)
 		die_perror("vhost-user kick eventfd_read()");
 
-	debug("vhost-user: got kick_data: %016"PRIx64" idx: %d",
+	trace("vhost-user: got kick_data: %016"PRIx64" idx: %d",
 	      kick_data, ref.queue);
 	if (VHOST_USER_IS_QUEUE_TX(ref.queue))
 		vu_handle_tx(vdev, ref.queue, now);
@@ -264,7 +264,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
 	int elem_cnt;
 	int i;
 
-	debug("vu_send_single size %zu", size);
+	trace("vu_send_single size %zu", size);
 
 	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
 		debug("Got packet, but RX virtqueue not usable yet");
@@ -296,7 +296,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
 
 	vu_flush(vdev, vq, elem, elem_cnt);
 
-	debug("vhost-user sent %zu", total);
+	trace("vhost-user sent %zu", total);
 
 	return total;
 err:
-- 
2.48.1

Re: [PATCH 1/6] vhost-user: Change different vhost-user messages to trace() level

[Stefano Brivio]

On Mon,  3 Feb 2025 20:26:10 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> A previous patch changed the debug() calls in vu_control_handler() to
> trace() to reduce noise when debugging migration.  However those messages
> are infrequent, and pretty useful when debugging migration (I used them to
> discover I needed a different qemu version).
> 
> At the same time we have potentially per-packet debug() messages in
> vu_kick_cb() and vu_send_single().  These get much more in the way since
> they occur asynchronously with the migration operation.  As a rule, per
> packet messages should be trace() level anyway.
> 
> [This is a candidate to fold into the earlier patch]
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Folded into existing patch and applied.

-- 
Stefano

[PATCH 2/6] migrate, flow: Abort migration on repair_flush() failure

[David Gibson]

If we're unable to set our sockets into repair mode, we won't be able to
complete our migration.  However flow_migrate_source_pre() currently
ignores errors from repair_flush().

Propagate the error instead.  Since we're not currently able to properly
roll back a partial migration setup this fails rather messily, but better
than carrying on as if nothing was wrong.

[This is a candidate to fold into the earlier patch]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/flow.c b/flow.c
index 8fcf8c44..ceb20183 100644
--- a/flow.c
+++ b/flow.c
@@ -898,7 +898,8 @@ int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m)
 			return rc;		/* TODO: rollback */
 	}
 
-	repair_flush(c);			/* TODO: move to TCP logic */
+	if ((rc = repair_flush(c)))		/* TODO: move to TCP logic */
+		return rc;
 
 	for (i = 0; i < FLOW_MAX; i++) {	/* TODO: iterator with skip */
 		union flow *flow = &flowtab[i];
-- 
@@ -898,7 +898,8 @@ int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m)
 			return rc;		/* TODO: rollback */
 	}
 
-	repair_flush(c);			/* TODO: move to TCP logic */
+	if ((rc = repair_flush(c)))		/* TODO: move to TCP logic */
+		return rc;
 
 	for (i = 0; i < FLOW_MAX; i++) {	/* TODO: iterator with skip */
 		union flow *flow = &flowtab[i];
-- 
2.48.1

[PATCH 3/6] migrate: Clearer debug message in migrate_request()

[David Gibson]

The message confusingly gives the *previous* value of c->device_state_fd as
the fd, not the newly assigned one.

[This is a candidate to fold into the earlier patch]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 migrate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migrate.c b/migrate.c
index d47c44b0..2d9f77ef 100644
--- a/migrate.c
+++ b/migrate.c
@@ -374,7 +374,8 @@ void migrate_close(struct ctx *c)
  */
 void migrate_request(struct ctx *c, int fd, bool target)
 {
-	debug("Migration requested, fd: %d", c->device_state_fd);
+	debug("Migration requested, fd: %d (was %d)",
+	      fd, c->device_state_fd);
 
 	if (c->device_state_fd != -1)
 		migrate_close(c);
-- 
@@ -374,7 +374,8 @@ void migrate_close(struct ctx *c)
  */
 void migrate_request(struct ctx *c, int fd, bool target)
 {
-	debug("Migration requested, fd: %d", c->device_state_fd);
+	debug("Migration requested, fd: %d (was %d)",
+	      fd, c->device_state_fd);
 
 	if (c->device_state_fd != -1)
 		migrate_close(c);
-- 
2.48.1

[PATCH 4/6] migrate: Handle sending header section from data sections

[David Gibson]

Currently the global state header is included in the list of data sections
to send for migration.  This makes for an asymmetry between the source and
target sides: for the source, the header is sent after the 'pre' handlers
along with all the rest of the data.  For the target side, the header must
be read first (before the 'pre' handlers), so that we know the version
which determines what all the rest of the data will be.

Change this so that the header is handled explicitly on both the source and
target side.  This will make some future changes simpler as well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 migrate.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/migrate.c b/migrate.c
index 2d9f77ef..025f3b71 100644
--- a/migrate.c
+++ b/migrate.c
@@ -50,7 +50,6 @@ static union migrate_header header = {
 
 /* Data sections for version 1 */
 static struct iovec sections_v1[] = {
-	{ &header,		sizeof(header) },
 	{ &flow_first_free,	sizeof(flow_first_free) },
 	{ flowtab,		sizeof(flowtab) },
 	{ flow_hashtab,		sizeof(flow_hashtab) },
@@ -172,6 +171,12 @@ static int migrate_source(struct ctx *c, int fd)
 	struct migrate_meta m;
 	int rc;
 
+	rc = write_all_buf(fd, &header, sizeof(header));
+	if (rc) {
+		err("Failed to send migration header: %s, abort", strerror_(rc));
+		return rc;
+	}
+
 	if ((rc = migrate_source_pre(c, &m))) {
 		err("Source pre-migration failed: %s, abort", strerror_(rc));
 		return rc;
@@ -279,10 +284,10 @@ static int migrate_target_state(int fd, const struct migrate_meta *m)
 
 	for (d = data_versions; d->v != m->v && d->v; d++);
 
-	for (cnt = 0; d->sections[cnt + 1 /* skip header */].iov_len; cnt++);
+	for (cnt = 0; d->sections[cnt].iov_len; cnt++);
 
 	debug("Reading %u migration sections", cnt);
-	rc = read_remainder(fd, d->sections + 1, cnt, 0);
+	rc = read_remainder(fd, d->sections, cnt, 0);
 	if (rc < 0)
 		return errno;
 
-- 
@@ -50,7 +50,6 @@ static union migrate_header header = {
 
 /* Data sections for version 1 */
 static struct iovec sections_v1[] = {
-	{ &header,		sizeof(header) },
 	{ &flow_first_free,	sizeof(flow_first_free) },
 	{ flowtab,		sizeof(flowtab) },
 	{ flow_hashtab,		sizeof(flow_hashtab) },
@@ -172,6 +171,12 @@ static int migrate_source(struct ctx *c, int fd)
 	struct migrate_meta m;
 	int rc;
 
+	rc = write_all_buf(fd, &header, sizeof(header));
+	if (rc) {
+		err("Failed to send migration header: %s, abort", strerror_(rc));
+		return rc;
+	}
+
 	if ((rc = migrate_source_pre(c, &m))) {
 		err("Source pre-migration failed: %s, abort", strerror_(rc));
 		return rc;
@@ -279,10 +284,10 @@ static int migrate_target_state(int fd, const struct migrate_meta *m)
 
 	for (d = data_versions; d->v != m->v && d->v; d++);
 
-	for (cnt = 0; d->sections[cnt + 1 /* skip header */].iov_len; cnt++);
+	for (cnt = 0; d->sections[cnt].iov_len; cnt++);
 
 	debug("Reading %u migration sections", cnt);
-	rc = read_remainder(fd, d->sections + 1, cnt, 0);
+	rc = read_remainder(fd, d->sections, cnt, 0);
 	if (rc < 0)
 		return errno;
 
-- 
2.48.1

[PATCH 5/6] util: read_remainder should take const pointer to iovec

[David Gibson]

read_remainder() takes a struct iovec * to describe where it will read its
data, unlike write_remainder() which takes a const struct iovec *.  At
first this seems like it makes sense, since read_remainder() will alter
data within the iovec.

However, what it actually alters is data within the buffers described by
the iovec, not the iovec entries itself.  So, like write it should take
a const struct iovec *.

[This is a candidate for folding into the earlier patch]

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 2 +-
 util.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index 0e0f8a4b..a9ea90a0 100644
--- a/util.c
+++ b/util.c
@@ -717,7 +717,7 @@ int read_all_buf(int fd, void *buf, size_t len)
  *
  * Note: mode-specific seccomp profiles need to enable readv() to use this.
  */
-int read_remainder(int fd, struct iovec *iov, size_t cnt, size_t skip)
+int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip)
 {
 	size_t i = 0, offset;
 
diff --git a/util.h b/util.h
index 6924d08d..dfac63b1 100644
--- a/util.h
+++ b/util.h
@@ -205,7 +205,7 @@ int write_file(const char *path, const char *buf);
 int write_all_buf(int fd, const void *buf, size_t len);
 int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
 int read_all_buf(int fd, void *buf, size_t len);
-int read_remainder(int fd, struct iovec *iov, size_t cnt, size_t skip);
+int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
 void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
 
-- 
@@ -205,7 +205,7 @@ int write_file(const char *path, const char *buf);
 int write_all_buf(int fd, const void *buf, size_t len);
 int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
 int read_all_buf(int fd, void *buf, size_t len);
-int read_remainder(int fd, struct iovec *iov, size_t cnt, size_t skip);
+int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
 void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
 
-- 
2.48.1

Re: [PATCH 5/6] util: read_remainder should take const pointer to iovec

[Stefano Brivio]

On Mon,  3 Feb 2025 20:26:14 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> read_remainder() takes a struct iovec * to describe where it will read its
> data, unlike write_remainder() which takes a const struct iovec *.  At
> first this seems like it makes sense, since read_remainder() will alter
> data within the iovec.
> 
> However, what it actually alters is data within the buffers described by
> the iovec, not the iovec entries itself.  So, like write it should take
> a const struct iovec *.
> 
> [This is a candidate for folding into the earlier patch]

Folded into existing patch and applied.

-- 
Stefano

[PATCH 6/6] migrate: Make migration handlers simpler and more flexible

[David Gibson]

Currently the structure of the migration callbacks is quite rigid.  There
are separate tables for pre and post handlers, for source and target.
Those only prepare or fix up with no reading or writing of the stream.

The actual reading and writing is done with an iovec of buffers to
transfer.  That can't handle any variably sized structures, nor sending
state which is only obtained during migration rather than being tracked
usually.

Replace both the handlers and the sections with an ordered list of
migration "stages".  Each stage has a callback for both source and target
side doing whatever is necessary - these can be NULL, for example for
preparation steps that have no equivalent on the other side.  Things that
are just buffers to be transferred are handled with a macro generating a
suitable stage entry.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c    |  14 ++-
 flow.h    |   6 +-
 migrate.c | 271 ++++++++++++++++++------------------------------------
 migrate.h |  54 ++++++-----
 4 files changed, 138 insertions(+), 207 deletions(-)

diff --git a/flow.c b/flow.c
index ceb20183..812b6a2b 100644
--- a/flow.c
+++ b/flow.c
@@ -875,15 +875,20 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now)
  * flow_migrate_source_pre() - Prepare all source flows for migration
  * @c:		Execution context
  * @m:		Migration metadata
+ * @stage:	Migration stage information (unused)
+ * @fd:		Migration fd (unused)
  *
  * Return: 0 on success
  */
-int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m)
+int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m,
+			    const struct migrate_stage *stage, int fd)
 {
 	unsigned i;
 	int rc;
 
 	(void)m;
+	(void)stage;
+	(void)fd;
 
 	for (i = 0; i < FLOW_MAX; i++) {	/* TODO: iterator with skip */
 		union flow *flow = &flowtab[i];
@@ -918,15 +923,20 @@ int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m)
  * flow_migrate_target_post() - Restore all flows after migration
  * @c:		Execution context
  * @m:		Migration metadata
+ * @stage:	Migration stage (unused)
+ * @fd:		Migration fd (unused)
  *
  * Return: 0 on success
  */
-int flow_migrate_target_post(struct ctx *c, struct migrate_meta *m)
+int flow_migrate_target_post(struct ctx *c, struct migrate_meta *m,
+			     const struct migrate_stage *stage, int fd)
 {
 	unsigned i;
 	int rc;
 
 	(void)m;
+	(void)stage;
+	(void)fd;
 
 	for (i = 0; i < FLOW_MAX; i++) {	/* TODO: iterator with skip */
 		union flow *flow = &flowtab[i];
diff --git a/flow.h b/flow.h
index 43fb5078..f1734008 100644
--- a/flow.h
+++ b/flow.h
@@ -255,8 +255,10 @@ union flow;
 
 void flow_init(void);
 void flow_defer_handler(const struct ctx *c, const struct timespec *now);
-int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m);
-int flow_migrate_target_post(struct ctx *c, struct migrate_meta *m);
+int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m,
+			    const struct migrate_stage *stage, int fd);
+int flow_migrate_target_post(struct ctx *c, struct migrate_meta *m,
+			     const struct migrate_stage *stage, int fd);
 
 void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...)
 	__attribute__((format(printf, 3, 4)));
diff --git a/migrate.c b/migrate.c
index 025f3b71..9f34eb58 100644
--- a/migrate.c
+++ b/migrate.c
@@ -48,116 +48,82 @@ static union migrate_header header = {
 	.voidp_size	= htonl_constant(sizeof(void *)),
 };
 
-/* Data sections for version 1 */
-static struct iovec sections_v1[] = {
-	{ &flow_first_free,	sizeof(flow_first_free) },
-	{ flowtab,		sizeof(flowtab) },
-	{ flow_hashtab,		sizeof(flow_hashtab) },
-	{ g_hash_secret,	sizeof(g_hash_secret) },
-	{ 0 },
-};
-
-/* Set of data versions */
-static struct migrate_data data_versions[] = {
-	{
-		1,	sections_v1,
-	},
-	{ 0 },
-};
-
-/* Handlers to call in source before sending data */
-struct migrate_handler handlers_source_pre[] = {
-	{ flow_migrate_source_pre },
-	{ 0 },
-};
-
-/* Handlers to call in source after sending data */
-struct migrate_handler handlers_source_post[] = {
-	{ 0 },
-};
-
-/* Handlers to call in target before receiving data with version 1 */
-struct migrate_handler handlers_target_pre_v1[] = {
-	{ 0 },
-};
-
-/* Handlers to call in target after receiving data with version 1 */
-struct migrate_handler handlers_target_post_v1[] = {
-	{ flow_migrate_target_post },
-	{ 0 },
-};
-
-/* Versioned sets of migration handlers */
-struct migrate_target_handlers target_handlers[] = {
-	{
-		1,
-		handlers_target_pre_v1,
-		handlers_target_post_v1,
-	},
-	{ 0 },
-};
-
 /**
- * migrate_source_pre() - Pre-migration tasks as source
+ * migrate_send_block() - Migration stage handler to send verbatim data
  * @c:		Execution context
  * @m:		Migration metadata
+ * @stage:	Migration stage
+ * @fd:		Migration fd
  *
- * Return: 0 on success, error code on failure
+ * Sends the buffer in @stage->iov over the migration channel.
  */
-static int migrate_source_pre(struct ctx *c, struct migrate_meta *m)
+static int migrate_send_block(struct ctx *c, struct migrate_meta *m,
+			      const struct migrate_stage *stage, int fd)
 {
-	struct migrate_handler *h;
-
-	for (h = handlers_source_pre; h->fn; h++) {
-		int rc;
+	(void)c;
+	(void)m;
 
-		if ((rc = h->fn(c, m)))
-			return rc;
-	}
+	if (write_remainder(fd, &stage->iov, 1, 0) < 0)
+		return errno;
 
 	return 0;
 }
 
 /**
- * migrate_source_state() - Send device state as migration source
- * @fd:		Descriptor for state transfer
+ * migrate_recv_block() - Migration stage handler to receive verbatim data
+ * @c:		Execution context
  * @m:		Migration metadata
+ * @stage:	Migration stage
+ * @fd:		Migration fd
  *
- * Return: 0 on success, error code on failure
+ * Reads the buffer in @stage->iov from the migration channel.
+ *
+ * #syscalls:vu readv
  */
-static int migrate_source_state(int fd, const struct migrate_meta *m)
+static int migrate_recv_block(struct ctx *c, struct migrate_meta *m,
+			      const struct migrate_stage *stage, int fd)
 {
-	static struct migrate_data *d;
-	int count, rc;
-
+	(void)c;
 	(void)m;
 
-	for (d = data_versions; d->v != MIGRATE_VERSION; d++);
-
-	for (count = 0; d->sections[count].iov_len; count++);
-
-	debug("Writing %u migration sections", count - 1 /* minus header */);
-	rc = write_remainder(fd, d->sections, count, 0);
-	if (rc < 0)
+	if (read_remainder(fd, &stage->iov, 1, 0) < 0)
 		return errno;
 
 	return 0;
 }
 
-/**
- * migrate_source_post() - Post-migration tasks as source
- * @c:		Execution context
- * @m:		Migration metadata
- *
- * Return: 0 on success, error code on failure
- */
-static void migrate_source_post(struct ctx *c, struct migrate_meta *m)
-{
-	struct migrate_handler *h;
+#define DATA_STAGE(v) \
+	{					\
+		.name = #v,			\
+		.source = migrate_send_block,	\
+		.target = migrate_recv_block,	\
+		.iov = { &(v), sizeof(v) },	\
+	}
 
-	for (h = handlers_source_post; h->fn; h++)
-		h->fn(c, m);
-}
+/* Stages for version 1 */
+static const struct migrate_stage stages_v1[] = {
+	{
+		.name = "flow pre",
+		.source = flow_migrate_source_pre,
+		.target = NULL,
+	},
+	DATA_STAGE(flow_first_free),
+	DATA_STAGE(flowtab),
+	DATA_STAGE(flow_hashtab),
+	DATA_STAGE(g_hash_secret),
+	{
+		.name = "flow post",
+		.source = NULL,
+		.target = flow_migrate_target_post,
+	},
+};
+
+/* Set of data versions */
+static const struct migrate_version versions[] = {
+	{
+		1,	stages_v1,	ARRAY_SIZE(stages_v1),
+	},
+};
 
 /**
  * migrate_source() - Migration as source, send state to hypervisor
@@ -169,6 +135,7 @@ static void migrate_source_post(struct ctx *c, struct migrate_meta *m)
 static int migrate_source(struct ctx *c, int fd)
 {
 	struct migrate_meta m;
+	unsigned i;
 	int rc;
 
 	rc = write_all_buf(fd, &header, sizeof(header));
@@ -177,20 +144,22 @@ static int migrate_source(struct ctx *c, int fd)
 		return rc;
 	}
 
-	if ((rc = migrate_source_pre(c, &m))) {
-		err("Source pre-migration failed: %s, abort", strerror_(rc));
-		return rc;
-	}
+	for (i = 0; i < ARRAY_SIZE(stages_v1); i++) {
+		const struct migrate_stage *stage = &stages_v1[i];
 
-	debug("Saving backend state");
+		if (!stage->source)
+			continue;
 
-	rc = migrate_source_state(fd, &m);
-	if (rc)
-		err("Source migration failed: %s", strerror_(rc));
-	else
-		migrate_source_post(c, &m);
+		debug("Source side migration: %s", stage->name);
 
-	return rc;
+		if ((rc = stage->source(c, &m, stage, fd))) {
+			err("Source migration failed stage %s: %s, abort",
+			    stage->name, strerror_(rc));
+			return rc;
+		}
+	}
+
+	return 0;
 }
 
 /**
@@ -202,8 +171,8 @@ static int migrate_source(struct ctx *c, int fd)
  */
 static int migrate_target_read_header(int fd, struct migrate_meta *m)
 {
-	static struct migrate_data *d;
 	union migrate_header h;
+	unsigned i;
 
 	if (read_all_buf(fd, &h, sizeof(h)))
 		return errno;
@@ -211,10 +180,14 @@ static int migrate_target_read_header(int fd, struct migrate_meta *m)
 	debug("Source magic: 0x%016" PRIx64 ", sizeof(void *): %u, version: %u",
 	      h.magic, ntohl(h.voidp_size), ntohl(h.version));
 
-	for (d = data_versions; d->v != ntohl(h.version) && d->v; d++);
-	if (!d->v)
+	m->version = ntohl(h.version);
+	m->v = NULL;
+	for (i = 0; i < ARRAY_SIZE(versions); i++) {
+		if (versions[i].version == m->version)
+			m->v = &versions[i];
+	}
+	if (!m->v)
 		return ENOTSUP;
-	m->v = d->v;
 
 	if (h.magic == MIGRATE_MAGIC)
 		m->bswap = false;
@@ -243,75 +216,6 @@ static int migrate_target_read_header(int fd, struct migrate_meta *m)
 	return 0;
 }
 
-/**
- * migrate_target_pre() - Pre-migration tasks as target
- * @c:		Execution context
- * @m:		Migration metadata
- *
- * Return: 0 on success, error code on failure
- */
-static int migrate_target_pre(struct ctx *c, struct migrate_meta *m)
-{
-	struct migrate_target_handlers *th;
-	struct migrate_handler *h;
-
-	for (th = target_handlers; th->v != m->v && th->v; th++);
-
-	for (h = th->pre; h->fn; h++) {
-		int rc;
-
-		if ((rc = h->fn(c, m)))
-			return rc;
-	}
-
-	return 0;
-}
-
-/**
- * migrate_target_state() - Receive device state as migration target
- * @fd:		Descriptor for state transfer
- * @m:		Migration metadata
- *
- * Return: 0 on success, error code on failure
- *
- * #syscalls:vu readv
- */
-static int migrate_target_state(int fd, const struct migrate_meta *m)
-{
-	static struct migrate_data *d;
-	unsigned cnt;
-	int rc;
-
-	for (d = data_versions; d->v != m->v && d->v; d++);
-
-	for (cnt = 0; d->sections[cnt].iov_len; cnt++);
-
-	debug("Reading %u migration sections", cnt);
-	rc = read_remainder(fd, d->sections, cnt, 0);
-	if (rc < 0)
-		return errno;
-
-	return 0;
-}
-
-/**
- * migrate_target_post() - Post-migration tasks as target
- * @c:		Execution context
- * @m:		Migration metadata
- */
-static void migrate_target_post(struct ctx *c, struct migrate_meta *m)
-{
-	struct migrate_target_handlers *th;
-	struct migrate_handler *h;
-
-	memcpy(c->hash_secret, g_hash_secret, sizeof(g_hash_secret));
-
-	for (th = target_handlers; th->v != m->v && th->v; th++);
-
-	for (h = th->post; h->fn; h++)
-		h->fn(c, m);
-}
-
 /**
  * migrate_target() - Migration as target, receive state from hypervisor
  * @c:		Execution context
@@ -322,6 +226,7 @@ static void migrate_target_post(struct ctx *c, struct migrate_meta *m)
 static int migrate_target(struct ctx *c, int fd)
 {
 	struct migrate_meta m;
+	unsigned i;
 	int rc;
 
 	rc = migrate_target_read_header(fd, &m);
@@ -330,20 +235,22 @@ static int migrate_target(struct ctx *c, int fd)
 		return rc;
 	}
 
-	if ((rc = migrate_target_pre(c, &m))) {
-		err("Target pre-migration failed: %s, abort", strerror_(rc));
-		return rc;
-	}
-
-	debug("Loading backend state");
+	for (i = 0; i < m.v->nstages; i++) {
+		const struct migrate_stage *stage = &m.v->stages[i];
 
-	rc = migrate_target_state(fd, &m);
-	if (rc)
-		err("Target migration failed: %s", strerror_(rc));
-	else
-		migrate_target_post(c, &m);
+		if (!stage->target)
+			continue;
 
-	return rc;
+		debug("Target side migration: %s", stage->name);
+		
+		if ((rc = stage->target(c, &m, stage, fd))) {
+			err("Target migration failed stage %s: %s, abort",
+			    stage->name, strerror_(rc));
+			return rc;
+		}
+	}
+	
+	return 0;
 }
 
 /**
diff --git a/migrate.h b/migrate.h
index 158241f3..32c0b3f4 100644
--- a/migrate.h
+++ b/migrate.h
@@ -6,9 +6,12 @@
 #ifndef MIGRATE_H
 #define MIGRATE_H
 
+struct migrate_version;
+
 /**
  * struct migrate_meta - Migration metadata
- * @v:			Chosen migration data version, host order
+ * @version:		Chosen migration data version, host order
+ * @v:			Migration version information
  * @bswap:		Source has opposite endianness
  * @peer_64b:		Source uses 64-bit void *
  * @time_64b:		Source uses 64-bit time_t
@@ -16,7 +19,8 @@
  * @flow_sidx_size:	Size of struct flow_sidx in source
  */
 struct migrate_meta {
-	uint32_t v;
+	uint32_t version;
+	const struct migrate_version *v;
 	bool bswap;
 	bool source_64b;
 	bool time_64b;
@@ -46,34 +50,42 @@ union migrate_header {
 	uint8_t unused[4096];
 } __attribute__((packed));
 
+struct migrate_stage;
+
 /**
- * struct migrate_data - Data sections for given source version
- * @v:			Source version this applies to, host order
- * @sections:		Array of data sections, NULL-terminated
+ * migrate_cb_t - Callback function to implement one migration stage
  */
-struct migrate_data {
-	uint32_t v;
-	struct iovec *sections;
-};
+typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
+			    const struct migrate_stage *stage, int fd);
 
 /**
- * struct migrate_handler - Function to handle a specific data section
- * @fn:			Function pointer taking pointer to context and metadata
+ * struct migrate_stage - Callbacks and parameters for one stage of migration
+ * @name:	Stage name (for debugging)
+ * @source:	Callback to implement this stage on the source
+ * @target:	Callback to implement this stage on the target
+ * @v:		Source version this applies to, host order
+ * @sections:	Array of data sections, NULL-terminated
  */
-struct migrate_handler {
-	int (*fn)(struct ctx *c, struct migrate_meta *m);
+struct migrate_stage {
+	const char *name;
+	migrate_cb_t source;
+	migrate_cb_t target;
+	/* FIXME: rollback callbacks? */
+	union {
+		struct iovec iov;
+	};
 };
 
 /**
- * struct migrate_target_handlers - Versioned sets of migration target handlers
- * @v:			Source version this applies to, host order
- * @pre:		Set of functions to execute in target before data copy
- * @post:		Set of functions to execute in target after data copy
+ * struct migrate_version - Stages for a particular protocol version
+ * @version:	Version number this implements
+ * @stages:	Ordered array of stages
+ * @nstages:	Length of @stages
  */
-struct migrate_target_handlers {
-	uint32_t v;
-	struct migrate_handler *pre;
-	struct migrate_handler *post;
+struct migrate_version {
+	uint32_t version;
+	const struct migrate_stage *stages;
+	unsigned nstages;
 };
 
 void migrate_init(struct ctx *c);
-- 
@@ -6,9 +6,12 @@
 #ifndef MIGRATE_H
 #define MIGRATE_H
 
+struct migrate_version;
+
 /**
  * struct migrate_meta - Migration metadata
- * @v:			Chosen migration data version, host order
+ * @version:		Chosen migration data version, host order
+ * @v:			Migration version information
  * @bswap:		Source has opposite endianness
  * @peer_64b:		Source uses 64-bit void *
  * @time_64b:		Source uses 64-bit time_t
@@ -16,7 +19,8 @@
  * @flow_sidx_size:	Size of struct flow_sidx in source
  */
 struct migrate_meta {
-	uint32_t v;
+	uint32_t version;
+	const struct migrate_version *v;
 	bool bswap;
 	bool source_64b;
 	bool time_64b;
@@ -46,34 +50,42 @@ union migrate_header {
 	uint8_t unused[4096];
 } __attribute__((packed));
 
+struct migrate_stage;
+
 /**
- * struct migrate_data - Data sections for given source version
- * @v:			Source version this applies to, host order
- * @sections:		Array of data sections, NULL-terminated
+ * migrate_cb_t - Callback function to implement one migration stage
  */
-struct migrate_data {
-	uint32_t v;
-	struct iovec *sections;
-};
+typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
+			    const struct migrate_stage *stage, int fd);
 
 /**
- * struct migrate_handler - Function to handle a specific data section
- * @fn:			Function pointer taking pointer to context and metadata
+ * struct migrate_stage - Callbacks and parameters for one stage of migration
+ * @name:	Stage name (for debugging)
+ * @source:	Callback to implement this stage on the source
+ * @target:	Callback to implement this stage on the target
+ * @v:		Source version this applies to, host order
+ * @sections:	Array of data sections, NULL-terminated
  */
-struct migrate_handler {
-	int (*fn)(struct ctx *c, struct migrate_meta *m);
+struct migrate_stage {
+	const char *name;
+	migrate_cb_t source;
+	migrate_cb_t target;
+	/* FIXME: rollback callbacks? */
+	union {
+		struct iovec iov;
+	};
 };
 
 /**
- * struct migrate_target_handlers - Versioned sets of migration target handlers
- * @v:			Source version this applies to, host order
- * @pre:		Set of functions to execute in target before data copy
- * @post:		Set of functions to execute in target after data copy
+ * struct migrate_version - Stages for a particular protocol version
+ * @version:	Version number this implements
+ * @stages:	Ordered array of stages
+ * @nstages:	Length of @stages
  */
-struct migrate_target_handlers {
-	uint32_t v;
-	struct migrate_handler *pre;
-	struct migrate_handler *post;
+struct migrate_version {
+	uint32_t version;
+	const struct migrate_stage *stages;
+	unsigned nstages;
 };
 
 void migrate_init(struct ctx *c);
-- 
2.48.1

Re: [PATCH 6/6] migrate: Make migration handlers simpler and more flexible

[Stefano Brivio]

On Mon,  3 Feb 2025 20:26:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> +/* Stages for version 1 */
> +static const struct migrate_stage stages_v1[] = {
> +	{
> +		.name = "flow pre",
> +		.source = flow_migrate_source_pre,
> +		.target = NULL,
> +	},
> +	DATA_STAGE(flow_first_free),
> +	DATA_STAGE(flowtab),
> +	DATA_STAGE(flow_hashtab),
> +	DATA_STAGE(g_hash_secret),

...so this one for g_hash_secret (which is the abomination I wanted
to drop) would eventually turn into a function?

It looks neat, I'm just not sure if we'll really have "data stages"
after I change the approach to only transfer the data we need as you
suggested.

Is that part of your pending queue by the way, or should I go ahead with
it?

> [...]
>
>  /**
> - * struct migrate_data - Data sections for given source version
> - * @v:			Source version this applies to, host order
> - * @sections:		Array of data sections, NULL-terminated
> + * migrate_cb_t - Callback function to implement one migration stage
>   */
> -struct migrate_data {
> -	uint32_t v;
> -	struct iovec *sections;
> -};
> +typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
> +			    const struct migrate_stage *stage, int fd);

typedef is really annoying, we already have a flow_sidx which kind of
made sense, but this has really no advantage over something like:

struct migrate_handler {
	int (*fn)(struct ctx *c, struct migrate_meta *m);
};

-- 
Stefano

Re: [PATCH 6/6] migrate: Make migration handlers simpler and more flexible

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On Mon, Feb 03, 2025 at 11:20:03AM +0100, Stefano Brivio wrote:
> On Mon,  3 Feb 2025 20:26:15 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > +/* Stages for version 1 */
> > +static const struct migrate_stage stages_v1[] = {
> > +	{
> > +		.name = "flow pre",
> > +		.source = flow_migrate_source_pre,
> > +		.target = NULL,
> > +	},
> > +	DATA_STAGE(flow_first_free),
> > +	DATA_STAGE(flowtab),
> > +	DATA_STAGE(flow_hashtab),
> > +	DATA_STAGE(g_hash_secret),
> 
> ...so this one for g_hash_secret (which is the abomination I wanted
> to drop) would eventually turn into a function?

Yes.

> It looks neat, I'm just not sure if we'll really have "data stages"
> after I change the approach to only transfer the data we need as you
> suggested.

I agree, that may well be the case, but we can just drop the macro and
helepr functions then.

> Is that part of your pending queue by the way, or should I go ahead with
> it?

Is which part of my pending queue?  g_hash_secret as a function?  No,
I've thought of it, but I haven't written it yet.

> 
> > [...]
> >
> >  /**
> > - * struct migrate_data - Data sections for given source version
> > - * @v:			Source version this applies to, host order
> > - * @sections:		Array of data sections, NULL-terminated
> > + * migrate_cb_t - Callback function to implement one migration stage
> >   */
> > -struct migrate_data {
> > -	uint32_t v;
> > -	struct iovec *sections;
> > -};
> > +typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
> > +			    const struct migrate_stage *stage, int fd);
> 
> typedef is really annoying, we already have a flow_sidx which kind of
> made sense, but this has really no advantage over something like:

Eh, I guess.  I find the extra . or -> a little annoying, but we can
do this if you'd prefer.

> struct migrate_handler {
> 	int (*fn)(struct ctx *c, struct migrate_meta *m);
> };
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] migrate: Make migration handlers simpler and more flexible

[Stefano Brivio]

On Tue, 4 Feb 2025 10:55:28 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 03, 2025 at 11:20:03AM +0100, Stefano Brivio wrote:
> > On Mon,  3 Feb 2025 20:26:15 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > +/* Stages for version 1 */
> > > +static const struct migrate_stage stages_v1[] = {
> > > +	{
> > > +		.name = "flow pre",
> > > +		.source = flow_migrate_source_pre,
> > > +		.target = NULL,
> > > +	},
> > > +	DATA_STAGE(flow_first_free),
> > > +	DATA_STAGE(flowtab),
> > > +	DATA_STAGE(flow_hashtab),
> > > +	DATA_STAGE(g_hash_secret),  
> > 
> > ...so this one for g_hash_secret (which is the abomination I wanted
> > to drop) would eventually turn into a function?  
> 
> Yes.
> 
> > It looks neat, I'm just not sure if we'll really have "data stages"
> > after I change the approach to only transfer the data we need as you
> > suggested.  
> 
> I agree, that may well be the case, but we can just drop the macro and
> helepr functions then.
> 
> > Is that part of your pending queue by the way, or should I go ahead with
> > it?  
> 
> Is which part of my pending queue?  g_hash_secret as a function?  No,
> I've thought of it, but I haven't written it yet.

No, I meant: "transfer the data we need".

> > > [...]
> > >
> > >  /**
> > > - * struct migrate_data - Data sections for given source version
> > > - * @v:			Source version this applies to, host order
> > > - * @sections:		Array of data sections, NULL-terminated
> > > + * migrate_cb_t - Callback function to implement one migration stage
> > >   */
> > > -struct migrate_data {
> > > -	uint32_t v;
> > > -	struct iovec *sections;
> > > -};
> > > +typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
> > > +			    const struct migrate_stage *stage, int fd);  
> > 
> > typedef is really annoying, we already have a flow_sidx which kind of
> > made sense, but this has really no advantage over something like:  
> 
> Eh, I guess.  I find the extra . or -> a little annoying, but we can
> do this if you'd prefer.

Well, I find not finding things because of the typedef quite annoying.

Actually, we don't even need a struct if you prefer (I actually do, but
I know not everybody finds the syntax for function pointers convenient).

I'm re-posting this squashed onto previous patches but it would be nice
if you could re-post a version of the whole thing without the typedef.

-- 
Stefano

Re: [PATCH 6/6] migrate: Make migration handlers simpler and more flexible

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

On Tue, Feb 04, 2025 at 01:12:09AM +0100, Stefano Brivio wrote:
> On Tue, 4 Feb 2025 10:55:28 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Feb 03, 2025 at 11:20:03AM +0100, Stefano Brivio wrote:
> > > On Mon,  3 Feb 2025 20:26:15 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > +/* Stages for version 1 */
> > > > +static const struct migrate_stage stages_v1[] = {
> > > > +	{
> > > > +		.name = "flow pre",
> > > > +		.source = flow_migrate_source_pre,
> > > > +		.target = NULL,
> > > > +	},
> > > > +	DATA_STAGE(flow_first_free),
> > > > +	DATA_STAGE(flowtab),
> > > > +	DATA_STAGE(flow_hashtab),
> > > > +	DATA_STAGE(g_hash_secret),  
> > > 
> > > ...so this one for g_hash_secret (which is the abomination I wanted
> > > to drop) would eventually turn into a function?  
> > 
> > Yes.
> > 
> > > It looks neat, I'm just not sure if we'll really have "data stages"
> > > after I change the approach to only transfer the data we need as you
> > > suggested.  
> > 
> > I agree, that may well be the case, but we can just drop the macro and
> > helepr functions then.
> > 
> > > Is that part of your pending queue by the way, or should I go ahead with
> > > it?  
> > 
> > Is which part of my pending queue?  g_hash_secret as a function?  No,
> > I've thought of it, but I haven't written it yet.
> 
> No, I meant: "transfer the data we need".

No, I haven't begun actually writing it.

> > > > [...]
> > > >
> > > >  /**
> > > > - * struct migrate_data - Data sections for given source version
> > > > - * @v:			Source version this applies to, host order
> > > > - * @sections:		Array of data sections, NULL-terminated
> > > > + * migrate_cb_t - Callback function to implement one migration stage
> > > >   */
> > > > -struct migrate_data {
> > > > -	uint32_t v;
> > > > -	struct iovec *sections;
> > > > -};
> > > > +typedef int (*migrate_cb_t)(struct ctx *c, struct migrate_meta *m,
> > > > +			    const struct migrate_stage *stage, int fd);  
> > > 
> > > typedef is really annoying, we already have a flow_sidx which kind of
> > > made sense, but this has really no advantage over something like:  
> > 
> > Eh, I guess.  I find the extra . or -> a little annoying, but we can
> > do this if you'd prefer.
> 
> Well, I find not finding things because of the typedef quite annoying.
> 
> Actually, we don't even need a struct if you prefer (I actually do, but
> I know not everybody finds the syntax for function pointers convenient).
> 
> I'm re-posting this squashed onto previous patches but it would be nice
> if you could re-post a version of the whole thing without the typedef.

Ok.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]