[PATCH 0/5] RFC: Assorted migration improvements

older
[PATCH] doc: Add mock of migration...

David Gibson

30 Jan 2025 30 Jan '25

9:33 a.m.

Here are some things that I think improve the draft migration logic. This is based on both the draft migration series (v2) and my epoll_del() series. Feel free to fold into the existing patches if that seems sensible. David Gibson (5): migrate: Add passt-repair to .gitignore migrate: vu_migrate_{source,target}() aren't actually vu speciic migrate: Move repair_sock_init() to vu_init() migrate: Make more handling common rather than vhost-user specific migrate: Don't handle the migration channel through epoll .gitignore | 1 + epoll_type.h | 2 - migrate.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++---- migrate.h | 12 ++--- passt.c | 6 +-- passt.h | 8 +++ tap.c | 3 -- vhost_user.c | 58 +++----------------- virtio.h | 4 -- vu_common.c | 89 ------------------------------- vu_common.h | 2 +- 11 files changed, 162 insertions(+), 171 deletions(-) -- 2.48.1

Show replies by date

David Gibson

30 Jan 30 Jan

9:33 a.m.

New subject: [PATCH 1/5] migrate: Add passt-repair to .gitignore

Like our other executable targets. Signed-off-by: David Gibson --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index d1c8be98..5824a71e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ /passt.avx2 /pasta /pasta.avx2 +/passt-repair /qrap /pasta.1 /seccomp.h -- 2.48.1

David Gibson

9:33 a.m.

New subject: [PATCH 2/5] migrate: vu_migrate_{source,target}() aren't actually vu speciic

vu_migrate_source() and vu_migrate_target() don't directly rely on anything vhost-user specific - it's just that they'll only be called for vhost-user so far. They are suitable as general top-level dispatchers for migration. Move them to migrate.c, rename to migrate_{source,target}() and make the lower-level functions they call local to migrate.c. Signed-off-by: David Gibson --- migrate.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++------ migrate.h | 10 ++----- vu_common.c | 66 ++----------------------------------------- 3 files changed, 75 insertions(+), 81 deletions(-) diff --git a/migrate.c b/migrate.c index 6707c029..148cd660 100644 --- a/migrate.c +++ b/migrate.c @@ -92,7 +92,7 @@ struct migrate_target_handlers target_handlers[] = { * * Return: 0 on success, error code on failure */ -int migrate_source_pre(struct ctx *c, struct migrate_meta *m) +static int migrate_source_pre(struct ctx *c, struct migrate_meta *m) { struct migrate_handler *h; @@ -107,13 +107,13 @@ int migrate_source_pre(struct ctx *c, struct migrate_meta *m) } /** - * migrate_source() - Perform migration as source: send state to hypervisor + * migrate_source_state() - Send device state as migration source * @fd: Descriptor for state transfer * @m: Migration metadata * * Return: 0 on success, error code on failure */ -int migrate_source(int fd, const struct migrate_meta *m) +static int migrate_source_state(int fd, const struct migrate_meta *m) { static struct migrate_data *d; int count, rc; @@ -139,7 +139,7 @@ int migrate_source(int fd, const struct migrate_meta *m) * * Return: 0 on success, error code on failure */ -void migrate_source_post(struct ctx *c, struct migrate_meta *m) +static void migrate_source_post(struct ctx *c, struct migrate_meta *m) { struct migrate_handler *h; @@ -147,6 +147,34 @@ void migrate_source_post(struct ctx *c, struct migrate_meta *m) h->fn(c, m); } +/** + * migrate_source() - Migration as source, send state to hypervisor + * @c: Execution context + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +int migrate_source(struct ctx *c, int fd) +{ + struct migrate_meta m; + int rc; + + if ((rc = migrate_source_pre(c, &m))) { + err("Source pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Saving backend state"); + + rc = migrate_source_state(fd, &m); + if (rc) + err("Source migration failed: %s", strerror_(rc)); + else + migrate_source_post(c, &m); + + return rc; +} + /** * migrate_target_read_header() - Set metadata in target from source header * @fd: Descriptor for state transfer @@ -154,7 +182,7 @@ void migrate_source_post(struct ctx *c, struct migrate_meta *m) * * Return: 0 on success, error code on failure */ -int migrate_target_read_header(int fd, struct migrate_meta *m) +static int migrate_target_read_header(int fd, struct migrate_meta *m) { static struct migrate_data *d; union migrate_header h; @@ -204,7 +232,7 @@ int migrate_target_read_header(int fd, struct migrate_meta *m) * * Return: 0 on success, error code on failure */ -int migrate_target_pre(struct ctx *c, struct migrate_meta *m) +static int migrate_target_pre(struct ctx *c, struct migrate_meta *m) { struct migrate_target_handlers *th; struct migrate_handler *h; @@ -222,7 +250,7 @@ int migrate_target_pre(struct ctx *c, struct migrate_meta *m) } /** - * migrate_target() - Perform migration as target: receive state from hypervisor + * migrate_target_state() - Receive device state as migration target * @fd: Descriptor for state transfer * @m: Migration metadata * @@ -230,7 +258,7 @@ int migrate_target_pre(struct ctx *c, struct migrate_meta *m) * * #syscalls:vu readv */ -int migrate_target(int fd, const struct migrate_meta *m) +static int migrate_target_state(int fd, const struct migrate_meta *m) { static struct migrate_data *d; unsigned cnt; @@ -253,7 +281,7 @@ int migrate_target(int fd, const struct migrate_meta *m) * @c: Execution context * @m: Migration metadata */ -void migrate_target_post(struct ctx *c, struct migrate_meta *m) +static void migrate_target_post(struct ctx *c, struct migrate_meta *m) { struct migrate_target_handlers *th; struct migrate_handler *h; @@ -263,3 +291,37 @@ void migrate_target_post(struct ctx *c, struct migrate_meta *m) for (h = th->post; h->fn; h++) h->fn(c, m); } + +/** + * migrate_target() - Migration as target, receive state from hypervisor + * @c: Execution context + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +int migrate_target(struct ctx *c, int fd) +{ + struct migrate_meta m; + int rc; + + rc = migrate_target_read_header(fd, &m); + if (rc) { + err("Migration header check failed: %s, abort", strerror_(rc)); + return rc; + } + + if ((rc = migrate_target_pre(c, &m))) { + err("Target pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Loading backend state"); + + rc = migrate_target_state(fd, &m); + if (rc) + err("Target migration failed: %s", strerror_(rc)); + else + migrate_target_post(c, &m); + + return rc; +} diff --git a/migrate.h b/migrate.h index f9635ac2..edf6303e 100644 --- a/migrate.h +++ b/migrate.h @@ -76,13 +76,7 @@ struct migrate_target_handlers { struct migrate_handler *post; }; -int migrate_source_pre(struct ctx *c, struct migrate_meta *m); -int migrate_source(int fd, const struct migrate_meta *m); -void migrate_source_post(struct ctx *c, struct migrate_meta *m); - -int migrate_target_read_header(int fd, struct migrate_meta *m); -int migrate_target_pre(struct ctx *c, struct migrate_meta *m); -int migrate_target(int fd, const struct migrate_meta *m); -void migrate_target_post(struct ctx *c, struct migrate_meta *m); +int migrate_source(struct ctx *c, int fd); +int migrate_target(struct ctx *c, int fd); #endif /* MIGRATE_H */ diff --git a/vu_common.c b/vu_common.c index c1980d99..d7e39e63 100644 --- a/vu_common.c +++ b/vu_common.c @@ -306,68 +306,6 @@ err: return -1; } -/** - * vu_migrate_source() - Migration as source, send state to hypervisor - * @c: Execution context - * @fd: File descriptor for state transfer - * - * Return: 0 on success, positive error code on failure - */ -static int vu_migrate_source(struct ctx *c, int fd) -{ - struct migrate_meta m; - int rc; - - if ((rc = migrate_source_pre(c, &m))) { - err("Source pre-migration failed: %s, abort", strerror_(rc)); - return rc; - } - - debug("Saving backend state"); - - rc = migrate_source(fd, &m); - if (rc) - err("Source migration failed: %s", strerror_(rc)); - else - migrate_source_post(c, &m); - - return rc; -} - -/** - * vu_migrate_target() - Migration as target, receive state from hypervisor - * @c: Execution context - * @fd: File descriptor for state transfer - * - * Return: 0 on success, positive error code on failure - */ -static int vu_migrate_target(struct ctx *c, int fd) -{ - struct migrate_meta m; - int rc; - - rc = migrate_target_read_header(fd, &m); - if (rc) { - err("Migration header check failed: %s, abort", strerror_(rc)); - return rc; - } - - if ((rc = migrate_target_pre(c, &m))) { - err("Target pre-migration failed: %s, abort", strerror_(rc)); - return rc; - } - - debug("Loading backend state"); - - rc = migrate_target(fd, &m); - if (rc) - err("Target migration failed: %s", strerror_(rc)); - else - migrate_target_post(c, &m); - - return rc; -} - /** * vu_migrate() - Send/receive passt internal state to/from QEMU * @c: Execution context @@ -381,9 +319,9 @@ void vu_migrate(struct ctx *c, uint32_t events) debug("vu_migrate fd %d events %x", vdev->device_state_fd, events); if (events & EPOLLOUT) - rc = vu_migrate_source(c, vdev->device_state_fd); + rc = migrate_source(c, vdev->device_state_fd); else if (events & EPOLLIN) - rc = vu_migrate_target(c, vdev->device_state_fd); + rc = migrate_target(c, vdev->device_state_fd); /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ -- 2.48.1

David Gibson

9:33 a.m.

New subject: [PATCH 3/5] migrate: Move repair_sock_init() to vu_init()

Currently we call repair_sock_init() immediately before tap_sock_unix_init(). However, this means it will be skipped if the vhost-user control fd is passed with --fd instead of being created at a specific path. We still need the repair socket in that case. Move it, instead, to vu_init(), which has the added advantage of moving all migration related one-time init to the same place. Signed-off-by: David Gibson --- tap.c | 3 --- vhost_user.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tap.c b/tap.c index 3659aabd..d1a9f522 100644 --- a/tap.c +++ b/tap.c @@ -56,7 +56,6 @@ #include "netlink.h" #include "pasta.h" #include "packet.h" -#include "repair.h" #include "tap.h" #include "log.h" #include "vhost_user.h" @@ -1362,8 +1361,6 @@ void tap_backend_init(struct ctx *c) tap_sock_tun_init(c); break; case MODE_VU: - repair_sock_init(c); - /* fall through */ case MODE_PASST: tap_sock_unix_init(c); diff --git a/vhost_user.c b/vhost_user.c index bbbf504f..5df29c47 100644 --- a/vhost_user.c +++ b/vhost_user.c @@ -44,6 +44,7 @@ #include "tap.h" #include "vhost_user.h" #include "pcap.h" +#include "repair.h" /* vhost-user version we are compatible with */ #define VHOST_USER_VERSION 1 @@ -1106,8 +1107,10 @@ void vu_init(struct ctx *c) } c->vdev->log_table = NULL; c->vdev->log_call_fd = -1; + c->vdev->device_state_fd = -1; c->vdev->device_state_result = -1; + repair_sock_init(c); } -- 2.48.1

Stefano Brivio

4 Feb 4 Feb

1:42 a.m.

New subject: [PATCH 3/5] migrate: Move repair_sock_init() to vu_init()

On Thu, 30 Jan 2025 19:33:28 +1100 David Gibson wrote:

...

Currently we call repair_sock_init() immediately before tap_sock_unix_init(). However, this means it will be skipped if the vhost-user control fd is passed with --fd instead of being created at a specific path.

That's intended, because we might not have a path in that case. See conf_open_files(). I know it's not perfectly consistent, but it's practical at the moment. The alternative opens up complicated questions such as: should we have a --fd-repair option? What should be the default path if only --fd is given? This is really unnecessary right now. -- Stefano

David Gibson

3:09 a.m.

New subject: [PATCH 3/5] migrate: Move repair_sock_init() to vu_init()

On Tue, Feb 04, 2025 at 01:42:31AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Jan 2025 19:33:28 +1100 David Gibson wrote:

...
Currently we call repair_sock_init() immediately before tap_sock_unix_init(). However, this means it will be skipped if the vhost-user control fd is passed with --fd instead of being created at a specific path.

That's intended, because we might not have a path in that case. See conf_open_files().

Hm, good point. We should probably make it conditional on actually having a path in that case. If --fd and --repair-path are used together, that should work, and at the moment it won't.

...

I know it's not perfectly consistent, but it's practical at the moment.

The alternative opens up complicated questions such as: should we have a --fd-repair option? What should be the default path if only --fd is given? This is really unnecessary right now.

I don't think we can have an --fd-repair, since part of the security model for passt-repair is that it connects to passt, not the other way around. We could, however, require --repair path if you want to use migration with --fd. -- 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

David Gibson

30 Jan 30 Jan

9:33 a.m.

New subject: [PATCH 4/5] migrate: Make more handling common rather than vhost-user specific

A lot of the migration logic in vhost_user.c and vu_common.c isn't really specific to vhost-user, but matches the overall structure of migration. This applies to vu_migrate() and to the parts of of vu_set_device_state_fd_exec() which aren't related to parsing the specific vhost-user control request. Move this logic to migrate.c, with matching renames. Signed-off-by: David Gibson --- epoll_type.h | 4 +-- migrate.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-- migrate.h | 6 ++-- passt.c | 6 ++-- passt.h | 6 ++++ vhost_user.c | 59 ++++----------------------------- virtio.h | 4 --- vu_common.c | 27 --------------- vu_common.h | 2 +- 9 files changed, 112 insertions(+), 94 deletions(-) diff --git a/epoll_type.h b/epoll_type.h index 706238aa..b981d30e 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -40,8 +40,8 @@ enum epoll_type { EPOLL_TYPE_VHOST_CMD, /* vhost-user kick event socket */ EPOLL_TYPE_VHOST_KICK, - /* vhost-user migration socket */ - EPOLL_TYPE_VHOST_MIGRATION, + /* migration device state channel */ + EPOLL_TYPE_DEVICE_STATE, /* TCP_REPAIR helper listening socket */ EPOLL_TYPE_REPAIR_LISTEN, /* TCP_REPAIR helper socket */ diff --git a/migrate.c b/migrate.c index 148cd660..04df6822 100644 --- a/migrate.c +++ b/migrate.c @@ -21,6 +21,7 @@ #include "inany.h" #include "flow.h" #include "flow_table.h" +#include "repair.h" #include "migrate.h" @@ -154,7 +155,7 @@ static void migrate_source_post(struct ctx *c, struct migrate_meta *m) * * Return: 0 on success, positive error code on failure */ -int migrate_source(struct ctx *c, int fd) +static int migrate_source(struct ctx *c, int fd) { struct migrate_meta m; int rc; @@ -299,7 +300,7 @@ static void migrate_target_post(struct ctx *c, struct migrate_meta *m) * * Return: 0 on success, positive error code on failure */ -int migrate_target(struct ctx *c, int fd) +static int migrate_target(struct ctx *c, int fd) { struct migrate_meta m; int rc; @@ -325,3 +326,90 @@ int migrate_target(struct ctx *c, int fd) return rc; } + +/** + * set_migration_watch() - Add the migration file descriptor to epoll + * @c: Execution context + * @fd: File descriptor to add + * @target: Are we the target of the migration? + */ +static void set_migration_watch(const struct ctx *c, int fd, bool target) +{ + union epoll_ref ref = { + .type = EPOLL_TYPE_DEVICE_STATE, + .fd = fd, + }; + struct epoll_event ev = { 0 }; + + ev.data.u64 = ref.u64; + ev.events = target ? EPOLLIN : EPOLLOUT; + + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev); +} + +/** + * migrate_init() - Set up things necessary for migration + * @c: Execution context + */ +void migrate_init(struct ctx *c) +{ + c->device_state_fd = -1; + c->device_state_result = -1; + repair_sock_init(c); +} + +/** + * migrate_close() - Close migration channel + * @c: Execution context + */ +void migrate_close(struct ctx *c) +{ + if (c->device_state_fd != -1) { + debug("Closing migration channel, fd: %d", c->device_state_fd); + epoll_del(c, c->device_state_fd); + close(c->device_state_fd); + c->device_state_fd = -1; + c->device_state_result = -1; + } +} + +/** + * migrate_request() - Request a migration of device state + * @c: Execution context + * @fd: fd to transfer state + * @target: Are we the target of the migration? + */ +void migrate_request(struct ctx *c, int fd, bool target) +{ + debug("Migration requested, fd: %d", c->device_state_fd); + + if (c->device_state_fd != -1) + migrate_close(c); + + c->device_state_fd = fd; + set_migration_watch(c, c->device_state_fd, target); + +} + +/** + * migrate_handler() - Send/receive passt internal state to/from QEMU + * @c: Execution context + * @events: epoll events + */ +void migrate_handler(struct ctx *c, uint32_t events) +{ + int rc = EIO; + + debug("migrate_handler fd %d events %x", c->device_state_fd, events); + + if (events & EPOLLOUT) + rc = migrate_source(c, c->device_state_fd); + else if (events & EPOLLIN) + rc = migrate_target(c, c->device_state_fd); + + /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ + + migrate_close(c); + + c->device_state_result = rc; +} diff --git a/migrate.h b/migrate.h index edf6303e..b97e55e7 100644 --- a/migrate.h +++ b/migrate.h @@ -76,7 +76,9 @@ struct migrate_target_handlers { struct migrate_handler *post; }; -int migrate_source(struct ctx *c, int fd); -int migrate_target(struct ctx *c, int fd); +void migrate_init(struct ctx *c); +void migrate_close(struct ctx *c); +void migrate_request(struct ctx *c, int fd, bool target); +void migrate_handler(struct ctx *c, uint32_t events); #endif /* MIGRATE_H */ diff --git a/passt.c b/passt.c index 1fa2ddd2..3c3a3313 100644 --- a/passt.c +++ b/passt.c @@ -76,7 +76,7 @@ char *epoll_type_str[] = { [EPOLL_TYPE_TAP_LISTEN] = "listening qemu socket", [EPOLL_TYPE_VHOST_CMD] = "vhost-user command socket", [EPOLL_TYPE_VHOST_KICK] = "vhost-user kick socket", - [EPOLL_TYPE_VHOST_MIGRATION] = "vhost-user migration socket", + [EPOLL_TYPE_DEVICE_STATE] = "migration device state channel", [EPOLL_TYPE_REPAIR_LISTEN] = "TCP_REPAIR helper listening socket", [EPOLL_TYPE_REPAIR] = "TCP_REPAIR helper socket", }; @@ -360,8 +360,8 @@ loop: case EPOLL_TYPE_VHOST_KICK: vu_kick_cb(c.vdev, ref, &now); break; - case EPOLL_TYPE_VHOST_MIGRATION: - vu_migrate(&c, eventmask); + case EPOLL_TYPE_DEVICE_STATE: + migrate_handler(&c, eventmask); break; case EPOLL_TYPE_REPAIR_LISTEN: repair_listen_handler(&c, eventmask); diff --git a/passt.h b/passt.h index 85b0a10f..5992cbeb 100644 --- a/passt.h +++ b/passt.h @@ -239,6 +239,8 @@ struct ip6_ctx { * @low_wmem: Low probed net.core.wmem_max * @low_rmem: Low probed net.core.rmem_max * @vdev: vhost-user device + * @device_state_fd: Device state migration channel + * @device_state_result: Device state migration result */ struct ctx { enum passt_modes mode; @@ -307,6 +309,10 @@ struct ctx { int low_rmem; struct vu_dev *vdev; + + /* Migration */ + int device_state_fd; + int device_state_result; }; void proto_update_l2_buf(const unsigned char *eth_d, diff --git a/vhost_user.c b/vhost_user.c index 5df29c47..2dde405a 100644 --- a/vhost_user.c +++ b/vhost_user.c @@ -44,7 +44,6 @@ #include "tap.h" #include "vhost_user.h" #include "pcap.h" -#include "repair.h" /* vhost-user version we are compatible with */ #define VHOST_USER_VERSION 1 @@ -998,36 +997,6 @@ static bool vu_send_rarp_exec(struct vu_dev *vdev, return false; } -/** - * vu_set_migration_watch() - Add the migration file descriptor to epoll - * @vdev: vhost-user device - * @fd: File descriptor to add - * @direction: Direction of the migration (save or load backend state) - */ -static void vu_set_migration_watch(const struct vu_dev *vdev, int fd, - uint32_t direction) -{ - union epoll_ref ref = { - .type = EPOLL_TYPE_VHOST_MIGRATION, - .fd = fd, - }; - struct epoll_event ev = { 0 }; - - ev.data.u64 = ref.u64; - switch (direction) { - case VHOST_USER_TRANSFER_STATE_DIRECTION_SAVE: - ev.events = EPOLLOUT; - break; - case VHOST_USER_TRANSFER_STATE_DIRECTION_LOAD: - ev.events = EPOLLIN; - break; - default: - ASSERT(0); - } - - epoll_ctl(vdev->context->epollfd, EPOLL_CTL_ADD, ref.fd, &ev); -} - /** * vu_set_device_state_fd_exec() - Set the device state migration channel * @vdev: vhost-user device @@ -1052,16 +1021,8 @@ static bool vu_set_device_state_fd_exec(struct vu_dev *vdev, direction != VHOST_USER_TRANSFER_STATE_DIRECTION_LOAD) die("Invalide device_state_fd direction: %d", direction); - if (vdev->device_state_fd != -1) { - epoll_del(vdev->context, vdev->device_state_fd); - close(vdev->device_state_fd); - } - - vdev->device_state_fd = msg->fds[0]; - vdev->device_state_result = -1; - vu_set_migration_watch(vdev, vdev->device_state_fd, direction); - - debug("Got device_state_fd: %d", vdev->device_state_fd); + migrate_request(vdev->context, msg->fds[0], + direction == VHOST_USER_TRANSFER_STATE_DIRECTION_LOAD); /* We don't provide a new fd for the data transfer */ vmsg_set_reply_u64(msg, VHOST_USER_VRING_NOFD_MASK); @@ -1079,9 +1040,7 @@ static bool vu_set_device_state_fd_exec(struct vu_dev *vdev, static bool vu_check_device_state_exec(struct vu_dev *vdev, struct vhost_user_msg *msg) { - (void)vdev; - - vmsg_set_reply_u64(msg, vdev->device_state_result); + vmsg_set_reply_u64(msg, vdev->context->device_state_result); return true; } @@ -1108,9 +1067,7 @@ void vu_init(struct ctx *c) c->vdev->log_table = NULL; c->vdev->log_call_fd = -1; - c->vdev->device_state_fd = -1; - c->vdev->device_state_result = -1; - repair_sock_init(c); + migrate_init(c); } @@ -1160,12 +1117,8 @@ void vu_cleanup(struct vu_dev *vdev) vu_close_log(vdev); - if (vdev->device_state_fd != -1) { - epoll_del(vdev->context, vdev->device_state_fd); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; - vdev->device_state_result = -1; - } + /* If we lose the VU dev, we also lose our migration channel */ + migrate_close(vdev->context); } /** diff --git a/virtio.h b/virtio.h index 7bef2d27..0a59441b 100644 --- a/virtio.h +++ b/virtio.h @@ -106,8 +106,6 @@ struct vu_dev_region { * @log_call_fd: Eventfd to report logging update * @log_size: Size of the logging memory region * @log_table: Base of the logging memory region - * @device_state_fd: Device state migration channel - * @device_state_result: Device state migration result */ struct vu_dev { struct ctx *context; @@ -119,8 +117,6 @@ struct vu_dev { int log_call_fd; uint64_t log_size; uint8_t *log_table; - int device_state_fd; - int device_state_result; }; /** diff --git a/vu_common.c b/vu_common.c index d7e39e63..78d1c1ba 100644 --- a/vu_common.c +++ b/vu_common.c @@ -305,30 +305,3 @@ err: return -1; } - -/** - * vu_migrate() - Send/receive passt internal state to/from QEMU - * @c: Execution context - * @events: epoll events - */ -void vu_migrate(struct ctx *c, uint32_t events) -{ - struct vu_dev *vdev = c->vdev; - int rc = EIO; - - debug("vu_migrate fd %d events %x", vdev->device_state_fd, events); - - if (events & EPOLLOUT) - rc = migrate_source(c, vdev->device_state_fd); - else if (events & EPOLLIN) - rc = migrate_target(c, vdev->device_state_fd); - - /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ - - vdev->device_state_result = rc; - - epoll_del(c, vdev->device_state_fd); - debug("Closing migration channel"); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; -} diff --git a/vu_common.h b/vu_common.h index 69c40063..f538f237 100644 --- a/vu_common.h +++ b/vu_common.h @@ -57,5 +57,5 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq, void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, const struct timespec *now); int vu_send_single(const struct ctx *c, const void *buf, size_t size); -void vu_migrate(struct ctx *c, uint32_t events); + #endif /* VU_COMMON_H */ -- 2.48.1

David Gibson

9:33 a.m.

New subject: [PATCH 5/5] migrate: Don't handle the migration channel through epoll

Currently, once a migration device state fd is assigned, we wait for EPOLLIN or EPOLLOUT events on it to actually perform the migration. Change it so that once a migration is requested it we complete it synchronously at the end of the current epoll cycle. This has several advantages: 1. It makes it clear that everything about the migration must be dealt with at once, not split between multiple epoll events on the channel 2. It ensures the migration always takes place between epoll cycles, rather than, for example, between handling TCP events and their deferred handling in post_handler(). 3. It reduces code setting up the epoll watch on the fd. Signed-off-by: David Gibson --- epoll_type.h | 2 -- migrate.c | 42 +++++++++++------------------------------- migrate.h | 2 +- passt.c | 6 ++---- passt.h | 2 ++ 5 files changed, 16 insertions(+), 38 deletions(-) diff --git a/epoll_type.h b/epoll_type.h index b981d30e..7f2a1217 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -40,8 +40,6 @@ enum epoll_type { EPOLL_TYPE_VHOST_CMD, /* vhost-user kick event socket */ EPOLL_TYPE_VHOST_KICK, - /* migration device state channel */ - EPOLL_TYPE_DEVICE_STATE, /* TCP_REPAIR helper listening socket */ EPOLL_TYPE_REPAIR_LISTEN, /* TCP_REPAIR helper socket */ diff --git a/migrate.c b/migrate.c index 04df6822..4279d1f8 100644 --- a/migrate.c +++ b/migrate.c @@ -327,26 +327,6 @@ static int migrate_target(struct ctx *c, int fd) return rc; } -/** - * set_migration_watch() - Add the migration file descriptor to epoll - * @c: Execution context - * @fd: File descriptor to add - * @target: Are we the target of the migration? - */ -static void set_migration_watch(const struct ctx *c, int fd, bool target) -{ - union epoll_ref ref = { - .type = EPOLL_TYPE_DEVICE_STATE, - .fd = fd, - }; - struct epoll_event ev = { 0 }; - - ev.data.u64 = ref.u64; - ev.events = target ? EPOLLIN : EPOLLOUT; - - epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev); -} - /** * migrate_init() - Set up things necessary for migration * @c: Execution context @@ -387,27 +367,27 @@ void migrate_request(struct ctx *c, int fd, bool target) migrate_close(c); c->device_state_fd = fd; - set_migration_watch(c, c->device_state_fd, target); - + c->migrate_target = target; } /** * migrate_handler() - Send/receive passt internal state to/from QEMU * @c: Execution context - * @events: epoll events */ -void migrate_handler(struct ctx *c, uint32_t events) +void migrate_handler(struct ctx *c) { - int rc = EIO; + int rc; - debug("migrate_handler fd %d events %x", c->device_state_fd, events); + if (c->device_state_fd < 0) + return; - if (events & EPOLLOUT) - rc = migrate_source(c, c->device_state_fd); - else if (events & EPOLLIN) - rc = migrate_target(c, c->device_state_fd); + debug("migrate_handler fd %d target %d", + c->device_state_fd, c->migrate_target); - /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ + if (c->migrate_target) + rc = migrate_target(c, c->device_state_fd); + else + rc = migrate_source(c, c->device_state_fd); migrate_close(c); diff --git a/migrate.h b/migrate.h index b97e55e7..3432940d 100644 --- a/migrate.h +++ b/migrate.h @@ -79,6 +79,6 @@ struct migrate_target_handlers { void migrate_init(struct ctx *c); void migrate_close(struct ctx *c); void migrate_request(struct ctx *c, int fd, bool target); -void migrate_handler(struct ctx *c, uint32_t events); +void migrate_handler(struct ctx *c); #endif /* MIGRATE_H */ diff --git a/passt.c b/passt.c index 3c3a3313..1938290f 100644 --- a/passt.c +++ b/passt.c @@ -76,7 +76,6 @@ char *epoll_type_str[] = { [EPOLL_TYPE_TAP_LISTEN] = "listening qemu socket", [EPOLL_TYPE_VHOST_CMD] = "vhost-user command socket", [EPOLL_TYPE_VHOST_KICK] = "vhost-user kick socket", - [EPOLL_TYPE_DEVICE_STATE] = "migration device state channel", [EPOLL_TYPE_REPAIR_LISTEN] = "TCP_REPAIR helper listening socket", [EPOLL_TYPE_REPAIR] = "TCP_REPAIR helper socket", }; @@ -360,9 +359,6 @@ loop: case EPOLL_TYPE_VHOST_KICK: vu_kick_cb(c.vdev, ref, &now); break; - case EPOLL_TYPE_DEVICE_STATE: - migrate_handler(&c, eventmask); - break; case EPOLL_TYPE_REPAIR_LISTEN: repair_listen_handler(&c, eventmask); break; @@ -377,5 +373,7 @@ loop: post_handler(&c, &now); + migrate_handler(&c); + goto loop; } diff --git a/passt.h b/passt.h index 5992cbeb..4189a4a5 100644 --- a/passt.h +++ b/passt.h @@ -241,6 +241,7 @@ struct ip6_ctx { * @vdev: vhost-user device * @device_state_fd: Device state migration channel * @device_state_result: Device state migration result + * @migrate_target: Is this the target for next migration? */ struct ctx { enum passt_modes mode; @@ -313,6 +314,7 @@ struct ctx { /* Migration */ int device_state_fd; int device_state_result; + bool migrate_target; }; void proto_update_l2_buf(const unsigned char *eth_d, -- 2.48.1

Stefano Brivio

31 Jan 31 Jan

6:37 a.m.

New subject: [PATCH 5/5] migrate: Don't handle the migration channel through epoll

On Thu, 30 Jan 2025 19:33:30 +1100 David Gibson wrote:

...

--- a/migrate.c +++ b/migrate.c @@ -327,26 +327,6 @@ static int migrate_target(struct ctx *c, int fd) return rc; }

-/** - * set_migration_watch() - Add the migration file descriptor to epoll - * @c: Execution context - * @fd: File descriptor to add - * @target: Are we the target of the migration? - */ -static void set_migration_watch(const struct ctx *c, int fd, bool target) -{ - union epoll_ref ref = { - .type = EPOLL_TYPE_DEVICE_STATE, - .fd = fd, - }; - struct epoll_event ev = { 0 }; - - ev.data.u64 = ref.u64; - ev.events = target ? EPOLLIN : EPOLLOUT; - - epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev);

This change should have dropped: epoll_del(c, c->device_state_fd); from migrate_close(). -- Stefano

David Gibson

7:23 a.m.

New subject: [PATCH 5/5] migrate: Don't handle the migration channel through epoll

On Fri, Jan 31, 2025 at 06:37:09AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Jan 2025 19:33:30 +1100 David Gibson wrote:

...
--- a/migrate.c +++ b/migrate.c @@ -327,26 +327,6 @@ static int migrate_target(struct ctx *c, int fd) return rc; }

-/** - * set_migration_watch() - Add the migration file descriptor to epoll - * @c: Execution context - * @fd: File descriptor to add - * @target: Are we the target of the migration? - */ -static void set_migration_watch(const struct ctx *c, int fd, bool target) -{ - union epoll_ref ref = { - .type = EPOLL_TYPE_DEVICE_STATE, - .fd = fd, - }; - struct epoll_event ev = { 0 }; - - ev.data.u64 = ref.u64; - ev.events = target ? EPOLLIN : EPOLLOUT; - - epoll_ctl(c->epollfd, EPOLL_CTL_ADD, ref.fd, &ev);

This change should have dropped:

epoll_del(c, c->device_state_fd);

from migrate_close().

Oops, yes. I've fixed it in my local version, in case I need to rebase before you've folded it all in. -- 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

Stefano Brivio

30 Jan 30 Jan

9:46 a.m.

On Thu, 30 Jan 2025 19:33:25 +1100 David Gibson wrote:

...

Here are some things that I think improve the draft migration logic. This is based on both the draft migration series (v2) and my epoll_del() series.

Feel free to fold into the existing patches if that seems sensible.

Sure, let me post a v3 with these included. Do you prefer to keep all the patches as they are or should I merge 2/5 and 3/5 with the patches in my current series? -- Stefano

David Gibson

9:55 a.m.

On Thu, Jan 30, 2025 at 09:46:19AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Jan 2025 19:33:25 +1100 David Gibson wrote:

...
Here are some things that I think improve the draft migration logic. This is based on both the draft migration series (v2) and my epoll_del() series.

Feel free to fold into the existing patches if that seems sensible.

Sure, let me post a v3 with these included.

Do you prefer to keep all the patches as they are or should I merge 2/5 and 3/5 with the patches in my current series?

Whichever you prefer. And 1/5 probably _should_ be folded in. -- 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

Stefano Brivio

31 Jan 31 Jan

6:46 a.m.

On Thu, 30 Jan 2025 19:55:11 +1100 David Gibson wrote:

...

On Thu, Jan 30, 2025 at 09:46:19AM +0100, Stefano Brivio wrote:

...
On Thu, 30 Jan 2025 19:33:25 +1100 David Gibson wrote:

...
Here are some things that I think improve the draft migration logic. This is based on both the draft migration series (v2) and my epoll_del() series.

Feel free to fold into the existing patches if that seems sensible.

Sure, let me post a v3 with these included.

Do you prefer to keep all the patches as they are or should I merge 2/5 and 3/5 with the patches in my current series?

Whichever you prefer. And 1/5 probably _should_ be folded in.

So, yeah, I have a merged series ready, but I'm trying to solve the concrete problem I'm facing before posting it. Actually, merging clean-ups with no _intended_ impact on functionality, while trying to do this, isn't terribly helpful, but it didn't take that long. -- Stefano

David Gibson

7:24 a.m.

On Fri, Jan 31, 2025 at 06:46:25AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Jan 2025 19:55:11 +1100 David Gibson wrote:

...
On Thu, Jan 30, 2025 at 09:46:19AM +0100, Stefano Brivio wrote:

...
On Thu, 30 Jan 2025 19:33:25 +1100 David Gibson wrote:

...
Here are some things that I think improve the draft migration logic. This is based on both the draft migration series (v2) and my epoll_del() series.

Feel free to fold into the existing patches if that seems sensible.

Sure, let me post a v3 with these included.

Do you prefer to keep all the patches as they are or should I merge 2/5 and 3/5 with the patches in my current series?

Whichever you prefer. And 1/5 probably _should_ be folded in.

So, yeah, I have a merged series ready, but I'm trying to solve the concrete problem I'm facing before posting it.

Actually, merging clean-ups with no _intended_ impact on functionality, while trying to do this, isn't terribly helpful, but it didn't take that long.

So, the cleanups aren't purely cleanups for the sake of cleanups: they're things I think I need in preparation for the more compact flow table representation stuff. -- 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

Age (days ago)

104

Last active (days ago)

List overview

Download

13 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH 0/5] RFC: Assorted migration improvements

tags

participants (2)