Re: [PATCH v3 14/20] migrate: Don't handle the migration channel through epoll

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Jan 31, 2025 at 08:39:47PM +0100, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> 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 <david@gibson.dropbear.id.au>
> ---
>  epoll_type.h |  2 --
>  migrate.c    | 44 +++++++++++---------------------------------
>  migrate.h    |  2 +-
>  passt.c      |  6 ++----
>  passt.h      |  2 ++
>  vu_common.c  | 27 +++++++++++++++++++++++++++
>  6 files changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/epoll_type.h b/epoll_type.h
> index b981d30..7f2a121 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 fc6a043..faa7841 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) },
>  };
>  
>  /* Set of data versions */
> @@ -333,26 +332,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
> @@ -372,7 +351,6 @@ 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;
> @@ -393,27 +371,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 a222c48..158241f 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 3c3a331..1938290 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 5992cbe..4189a4a 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,
> diff --git a/vu_common.c b/vu_common.c
> index 78d1c1b..4797ef9 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -305,3 +305,30 @@ 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_ctl(c->epollfd, EPOLL_CTL_DEL, vdev->device_state_fd, NULL);
> +	debug("Closing migration channel");
> +	close(vdev->device_state_fd);
> +	vdev->device_state_fd = -1;
> +}

Also, this hunk, restoring the function removed in an earlier patch
seems to have crept in here, which also breaks build.

-- 
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 --]