[RFC 0/3] Vhost: fix FD entries cleanup

[RFC 0/3] Vhost: fix FD entries cleanup

[Maxime Coquelin]

The vhost FD manager provides a way for the read/write
callbacks to request removal of their associated FD from
the epoll FD set. Problem is that it is missing a cleanup
callback, so the read/write callback requesting the removal
have to perform cleanups before the FD is removed from the
FD set. It includes closing the FD before it is removed
from the epoll FD set.

This series introduces a new cleanup callback which, if
implemented, is closed right after the FD is removed from
FD set.

Maxime Coquelin (3):
  vhost: add cleanup callback to FD entries
  vhost: fix vhost-user socket cleanup order
  vhost: improve VDUSE reconnect handler cleanup

 lib/vhost/fd_man.c | 16 ++++++++++++----
 lib/vhost/fd_man.h |  3 ++-
 lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
 lib/vhost/vduse.c  | 16 +++++++++++-----
 4 files changed, 51 insertions(+), 30 deletions(-)

-- 
2.47.1

[RFC 1/3] vhost: add cleanup callback to FD entries

[Maxime Coquelin]

This patch adds an optional cleanup callback to
FD entries in the FD manager, which is called by the
FD event thread at FD entry removal time (when the read
or write callback has requested the FD removal).

Doing this, we avoid closing the file descriptor before it
is removed from the epoll FD set.

Fixes: 0e38b42bf61c ("vhost: manage FD with epoll")
Cc: stable@dpdk.org

Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/fd_man.c | 16 ++++++++++++----
 lib/vhost/fd_man.h |  3 ++-
 lib/vhost/socket.c |  4 ++--
 lib/vhost/vduse.c  |  6 +++---
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
index 9bc7e50b93..c172ad8eff 100644
--- a/lib/vhost/fd_man.c
+++ b/lib/vhost/fd_man.c
@@ -25,6 +25,8 @@ struct fdentry {
 	int fd;		/* -1 indicates this entry is empty */
 	fd_cb rcb;	/* callback when this fd is readable. */
 	fd_cb wcb;	/* callback when this fd is writeable.*/
+	cleanup_cb ccb; /* callback when cleanup after entry
+			   removal from the fdset is needed */
 	void *dat;	/* fd context */
 	int busy;	/* whether this entry is being used in cb. */
 	LIST_ENTRY(fdentry) next;
@@ -150,7 +152,7 @@ fdset_init(const char *name)
 }
 
 static int
-fdset_insert_entry(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
+fdset_insert_entry(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, cleanup_cb ccb, void *dat)
 {
 	struct fdentry *pfdentry;
 
@@ -161,6 +163,7 @@ fdset_insert_entry(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat
 	pfdentry->fd  = fd;
 	pfdentry->rcb = rcb;
 	pfdentry->wcb = wcb;
+	pfdentry->ccb = ccb;
 	pfdentry->dat = dat;
 
 	LIST_INSERT_HEAD(&pfdset->fdlist, pfdentry, next);
@@ -210,7 +213,7 @@ fdset_find_entry_locked(struct fdset *pfdset, int fd)
  * Register the fd in the fdset with read/write handler and context.
  */
 int
-fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
+fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, cleanup_cb ccb, void *dat)
 {
 	struct epoll_event ev;
 	struct fdentry *pfdentry;
@@ -222,7 +225,7 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
 	}
 
 	pthread_mutex_lock(&pfdset->fd_mutex);
-	ret = fdset_insert_entry(pfdset, fd, rcb, wcb, dat);
+	ret = fdset_insert_entry(pfdset, fd, rcb, wcb, ccb, dat);
 	if (ret < 0) {
 		VHOST_FDMAN_LOG(ERR, "failed to insert fdset entry");
 		pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -331,6 +334,7 @@ fdset_event_dispatch(void *arg)
 {
 	int i;
 	fd_cb rcb, wcb;
+	cleanup_cb ccb;
 	void *dat;
 	int fd, numfds;
 	int remove1, remove2;
@@ -361,6 +365,7 @@ fdset_event_dispatch(void *arg)
 
 			rcb = pfdentry->rcb;
 			wcb = pfdentry->wcb;
+			ccb = pfdentry->ccb;
 			dat = pfdentry->dat;
 			pfdentry->busy = 1;
 
@@ -381,8 +386,11 @@ fdset_event_dispatch(void *arg)
 			 * fdentry not to be busy, so we can't call
 			 * fdset_del_locked().
 			 */
-			if (remove1 || remove2)
+			if (remove1 || remove2) {
 				fdset_del(pfdset, fd);
+				if (ccb)
+					ccb(fd, dat);
+			}
 		}
 
 		if (pfdset->destroy)
diff --git a/lib/vhost/fd_man.h b/lib/vhost/fd_man.h
index 6398343a6a..452c89135d 100644
--- a/lib/vhost/fd_man.h
+++ b/lib/vhost/fd_man.h
@@ -13,11 +13,12 @@ struct fdset;
 #define MAX_FDS 1024
 
 typedef void (*fd_cb)(int fd, void *dat, int *remove);
+typedef void (*cleanup_cb)(int fd, void *dat);
 
 struct fdset *fdset_init(const char *name);
 
 int fdset_add(struct fdset *pfdset, int fd,
-	fd_cb rcb, fd_cb wcb, void *dat);
+	fd_cb rcb, fd_cb wcb, cleanup_cb ccb, void *dat);
 
 void fdset_del(struct fdset *pfdset, int fd);
 int fdset_try_del(struct fdset *pfdset, int fd);
diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index d29d15494c..e301efb367 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -263,7 +263,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 	conn->vsocket = vsocket;
 	conn->vid = vid;
 	ret = fdset_add(vhost_user.fdset, fd, vhost_user_read_cb,
-			NULL, conn);
+			NULL, NULL, conn);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(vsocket->path, ERR,
 			"failed to add fd %d into vhost server fdset",
@@ -396,7 +396,7 @@ vhost_user_start_server(struct vhost_user_socket *vsocket)
 		goto err;
 
 	ret = fdset_add(vhost_user.fdset, fd, vhost_user_server_new_connection,
-		  NULL, vsocket);
+		  NULL, NULL, vsocket);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(path, ERR, "failed to add listen fd %d to vhost server fdset",
 			fd);
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 8ba58555f9..9c10fbc684 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -218,7 +218,7 @@ vduse_vring_setup(struct virtio_net *dev, unsigned int index, bool reconnect)
 	}
 
 	if (vq == dev->cvq) {
-		ret = fdset_add(vduse.fdset, vq->kickfd, vduse_control_queue_event, NULL, dev);
+		ret = fdset_add(vduse.fdset, vq->kickfd, vduse_control_queue_event, NULL, NULL, dev);
 		if (ret) {
 			VHOST_CONFIG_LOG(dev->ifname, ERR,
 					"Failed to setup kickfd handler for VQ %u: %s",
@@ -590,7 +590,7 @@ vduse_reconnect_start_device(struct virtio_net *dev)
 		goto out_err;
 	}
 
-	ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
+	ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, NULL, dev);
 	if (ret) {
 		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
 				fd);
@@ -787,7 +787,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 
 	dev->cvq = dev->virtqueue[max_queue_pairs * 2];
 
-	ret = fdset_add(vduse.fdset, dev->vduse_dev_fd, vduse_events_handler, NULL, dev);
+	ret = fdset_add(vduse.fdset, dev->vduse_dev_fd, vduse_events_handler, NULL, NULL, dev);
 	if (ret) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to add fd %d to vduse fdset",
 				dev->vduse_dev_fd);
-- 
2.47.1

[RFC 2/3] vhost: fix vhost-user socket cleanup order

[Maxime Coquelin]

This patch leverages the new FD entry cleanup callback
to properly cleanup the vhost-user socket on disconnection
from the frontend.

Fixes: 0e38b42bf61c ("vhost: manage FD with epoll")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/socket.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index e301efb367..898570606c 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -86,6 +86,7 @@ struct vhost_user {
 
 static void vhost_user_server_new_connection(int fd, void *data, int *remove);
 static void vhost_user_read_cb(int fd, void *dat, int *remove);
+static void vhost_user_cleanup_cb(int fd, void *dat);
 static int create_unix_socket(struct vhost_user_socket *vsocket);
 static int vhost_user_start_client(struct vhost_user_socket *vsocket);
 
@@ -263,7 +264,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 	conn->vsocket = vsocket;
 	conn->vid = vid;
 	ret = fdset_add(vhost_user.fdset, fd, vhost_user_read_cb,
-			NULL, NULL, conn);
+			NULL, vhost_user_cleanup_cb, conn);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(vsocket->path, ERR,
 			"failed to add fd %d into vhost server fdset",
@@ -306,35 +307,40 @@ static void
 vhost_user_read_cb(int connfd, void *dat, int *remove)
 {
 	struct vhost_user_connection *conn = dat;
-	struct vhost_user_socket *vsocket = conn->vsocket;
 	int ret;
 
 	ret = vhost_user_msg_handler(conn->vid, connfd);
-	if (ret < 0) {
-		struct virtio_net *dev = get_device(conn->vid);
-
-		close(connfd);
+	if (ret < 0)
 		*remove = 1;
+}
 
-		if (dev)
-			vhost_destroy_device_notify(dev);
+static void
+vhost_user_cleanup_cb(int connfd, void *dat)
+{
+	struct vhost_user_connection *conn = dat;
+	struct vhost_user_socket *vsocket = conn->vsocket;
+	struct virtio_net *dev = get_device(conn->vid);
 
-		if (vsocket->notify_ops->destroy_connection)
-			vsocket->notify_ops->destroy_connection(conn->vid);
+	close(connfd);
 
-		vhost_destroy_device(conn->vid);
+	if (dev)
+		vhost_destroy_device_notify(dev);
 
-		if (vsocket->reconnect) {
-			create_unix_socket(vsocket);
-			vhost_user_start_client(vsocket);
-		}
+	if (vsocket->notify_ops->destroy_connection)
+		vsocket->notify_ops->destroy_connection(conn->vid);
 
-		pthread_mutex_lock(&vsocket->conn_mutex);
-		TAILQ_REMOVE(&vsocket->conn_list, conn, next);
-		pthread_mutex_unlock(&vsocket->conn_mutex);
+	vhost_destroy_device(conn->vid);
 
-		free(conn);
+	if (vsocket->reconnect) {
+		create_unix_socket(vsocket);
+		vhost_user_start_client(vsocket);
 	}
+
+	pthread_mutex_lock(&vsocket->conn_mutex);
+	TAILQ_REMOVE(&vsocket->conn_list, conn, next);
+	pthread_mutex_unlock(&vsocket->conn_mutex);
+
+	free(conn);
 }
 
 static int
-- 
2.47.1

[RFC 3/3] vhost: improve VDUSE reconnect handler cleanup

[Maxime Coquelin]

This patch makes use of the new FD entry cleanup callback
to close the VDUSE reconnect eventfd after its removal
from the FD set.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 9c10fbc684..a841da966c 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -563,16 +563,21 @@ vduse_reconnect_log_check(struct virtio_net *dev, uint64_t features, uint32_t to
 }
 
 static void
-vduse_reconnect_handler(int fd, void *arg, int *remove)
+vduse_reconnect_handler(int fd __rte_unused, void *arg, int *remove)
 {
 	struct virtio_net *dev = arg;
 
 	vduse_device_start(dev, true);
 
-	close(fd);
 	*remove = 1;
 }
 
+static void
+vduse_reconnect_handler_cleanup(int fd, void *arg __rte_unused)
+{
+	close(fd);
+}
+
 static int
 vduse_reconnect_start_device(struct virtio_net *dev)
 {
@@ -590,7 +595,8 @@ vduse_reconnect_start_device(struct virtio_net *dev)
 		goto out_err;
 	}
 
-	ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, NULL, dev);
+	ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL,
+			vduse_reconnect_handler_cleanup, dev);
 	if (ret) {
 		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
 				fd);
-- 
2.47.1

Re: [RFC 0/3] Vhost: fix FD entries cleanup

[David Marchand]

Hello vhost maintainers,

On Tue, Dec 24, 2024 at 4:50 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> The vhost FD manager provides a way for the read/write
> callbacks to request removal of their associated FD from
> the epoll FD set. Problem is that it is missing a cleanup
> callback, so the read/write callback requesting the removal
> have to perform cleanups before the FD is removed from the
> FD set. It includes closing the FD before it is removed
> from the epoll FD set.
>
> This series introduces a new cleanup callback which, if
> implemented, is closed right after the FD is removed from
> FD set.
>
> Maxime Coquelin (3):
>   vhost: add cleanup callback to FD entries
>   vhost: fix vhost-user socket cleanup order
>   vhost: improve VDUSE reconnect handler cleanup
>
>  lib/vhost/fd_man.c | 16 ++++++++++++----
>  lib/vhost/fd_man.h |  3 ++-
>  lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
>  lib/vhost/vduse.c  | 16 +++++++++++-----
>  4 files changed, 51 insertions(+), 30 deletions(-)

I tried this series, and it fixes the error log I reported.

On the other hand, I wonder if we could do something simpler.

The fd is only used by the registered handlers.
If a handler reports that it does not want to watch this fd anymore,
then there is no remaining user in the vhost library for this fd.

So my proposal would be to rename the "remove" flag as a "close" flag:

@@ -12,7 +12,7 @@ struct fdset;

 #define MAX_FDS 1024

-typedef void (*fd_cb)(int fd, void *dat, int *remove);
+typedef void (*fd_cb)(int fd, void *dat, int *close);

 struct fdset *fdset_init(const char *name);

And defer closing to fd_man.
Something like:

@@ -367,9 +367,9 @@ fdset_event_dispatch(void *arg)
                        pthread_mutex_unlock(&pfdset->fd_mutex);

                        if (rcb && events[i].events & (EPOLLIN |
EPOLLERR | EPOLLHUP))
-                               rcb(fd, dat, &remove1);
+                               rcb(fd, dat, &close1);
                        if (wcb && events[i].events & (EPOLLOUT |
EPOLLERR | EPOLLHUP))
-                               wcb(fd, dat, &remove2);
+                               wcb(fd, dat, &close2);
                        pfdentry->busy = 0;
                        /*
                         * fdset_del needs to check busy flag.
@@ -381,8 +381,10 @@ fdset_event_dispatch(void *arg)
                         * fdentry not to be busy, so we can't call
                         * fdset_del_locked().
                         */
-                       if (remove1 || remove2)
+                       if (close1 || close2) {
                                fdset_del(pfdset, fd);
+                               close(fd);
+                       }
                }

                if (pfdset->destroy)


And the only thing to move out of the socket and vduse handlers is the
close(fd) call.

Like:

@@ -303,7 +303,7 @@ vhost_user_server_new_connection(int fd, void
*dat, int *remove __rte_unused)
 }

 static void
-vhost_user_read_cb(int connfd, void *dat, int *remove)
+vhost_user_read_cb(int connfd, void *dat, int *close)
 {
        struct vhost_user_connection *conn = dat;
        struct vhost_user_socket *vsocket = conn->vsocket;
@@ -313,8 +313,7 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
        if (ret < 0) {
                struct virtio_net *dev = get_device(conn->vid);

-               close(connfd);
-               *remove = 1;
+               *close = 1;

                if (dev)
                        vhost_destroy_device_notify(dev);


Maxime, Chenbo, opinions?


-- 
David Marchand

Re: [RFC 0/3] Vhost: fix FD entries cleanup

[Chenbo Xia]

Hi David,

> On Feb 4, 2025, at 21:18, David Marchand <david.marchand@redhat.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello vhost maintainers,
> 
> On Tue, Dec 24, 2024 at 4:50 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> 
>> The vhost FD manager provides a way for the read/write
>> callbacks to request removal of their associated FD from
>> the epoll FD set. Problem is that it is missing a cleanup
>> callback, so the read/write callback requesting the removal
>> have to perform cleanups before the FD is removed from the
>> FD set. It includes closing the FD before it is removed
>> from the epoll FD set.
>> 
>> This series introduces a new cleanup callback which, if
>> implemented, is closed right after the FD is removed from
>> FD set.
>> 
>> Maxime Coquelin (3):
>>  vhost: add cleanup callback to FD entries
>>  vhost: fix vhost-user socket cleanup order
>>  vhost: improve VDUSE reconnect handler cleanup
>> 
>> lib/vhost/fd_man.c | 16 ++++++++++++----
>> lib/vhost/fd_man.h |  3 ++-
>> lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
>> lib/vhost/vduse.c  | 16 +++++++++++-----
>> 4 files changed, 51 insertions(+), 30 deletions(-)
> 
> I tried this series, and it fixes the error log I reported.
> 
> On the other hand, I wonder if we could do something simpler.
> 
> The fd is only used by the registered handlers.
> If a handler reports that it does not want to watch this fd anymore,
> then there is no remaining user in the vhost library for this fd.
> 
> So my proposal would be to rename the "remove" flag as a "close" flag:
> 
> @@ -12,7 +12,7 @@ struct fdset;
> 
> #define MAX_FDS 1024
> 
> -typedef void (*fd_cb)(int fd, void *dat, int *remove);
> +typedef void (*fd_cb)(int fd, void *dat, int *close);
> 
> struct fdset *fdset_init(const char *name);
> 
> And defer closing to fd_man.
> Something like:
> 
> @@ -367,9 +367,9 @@ fdset_event_dispatch(void *arg)
>                        pthread_mutex_unlock(&pfdset->fd_mutex);
> 
>                        if (rcb && events[i].events & (EPOLLIN |
> EPOLLERR | EPOLLHUP))
> -                               rcb(fd, dat, &remove1);
> +                               rcb(fd, dat, &close1);
>                        if (wcb && events[i].events & (EPOLLOUT |
> EPOLLERR | EPOLLHUP))
> -                               wcb(fd, dat, &remove2);
> +                               wcb(fd, dat, &close2);
>                        pfdentry->busy = 0;
>                        /*
>                         * fdset_del needs to check busy flag.
> @@ -381,8 +381,10 @@ fdset_event_dispatch(void *arg)
>                         * fdentry not to be busy, so we can't call
>                         * fdset_del_locked().
>                         */
> -                       if (remove1 || remove2)
> +                       if (close1 || close2) {
>                                fdset_del(pfdset, fd);
> +                               close(fd);
> +                       }
>                }
> 
>                if (pfdset->destroy)
> 
> 
> And the only thing to move out of the socket and vduse handlers is the
> close(fd) call.
> 
> Like:
> 
> @@ -303,7 +303,7 @@ vhost_user_server_new_connection(int fd, void
> *dat, int *remove __rte_unused)
> }
> 
> static void
> -vhost_user_read_cb(int connfd, void *dat, int *remove)
> +vhost_user_read_cb(int connfd, void *dat, int *close)
> {
>        struct vhost_user_connection *conn = dat;
>        struct vhost_user_socket *vsocket = conn->vsocket;
> @@ -313,8 +313,7 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
>        if (ret < 0) {
>                struct virtio_net *dev = get_device(conn->vid);
> 
> -               close(connfd);
> -               *remove = 1;
> +               *close = 1;

I have one concern here is compared with this RFC, the proposal changed the timing
of close connfd，which means on QEMU side, cleaning up resources will happen later.

Currently I can’t think of issues could be introduced by this change (maybe you and
Maxime could remind me of something :)

Besides this, definitely this proposal is cleaner.

Thanks,
Chenbo 

> 
>                if (dev)
>                        vhost_destroy_device_notify(dev);
> 
> 
> Maxime, Chenbo, opinions?
> 
> 
> --
> David Marchand
> 

Re: [RFC 0/3] Vhost: fix FD entries cleanup

[Chenbo Xia]

Hi David,

> On Feb 4, 2025, at 21:18, David Marchand <david.marchand@redhat.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello vhost maintainers,
> 
> On Tue, Dec 24, 2024 at 4:50 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> 
>> The vhost FD manager provides a way for the read/write
>> callbacks to request removal of their associated FD from
>> the epoll FD set. Problem is that it is missing a cleanup
>> callback, so the read/write callback requesting the removal
>> have to perform cleanups before the FD is removed from the
>> FD set. It includes closing the FD before it is removed
>> from the epoll FD set.
>> 
>> This series introduces a new cleanup callback which, if
>> implemented, is closed right after the FD is removed from
>> FD set.
>> 
>> Maxime Coquelin (3):
>>  vhost: add cleanup callback to FD entries
>>  vhost: fix vhost-user socket cleanup order
>>  vhost: improve VDUSE reconnect handler cleanup
>> 
>> lib/vhost/fd_man.c | 16 ++++++++++++----
>> lib/vhost/fd_man.h |  3 ++-
>> lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
>> lib/vhost/vduse.c  | 16 +++++++++++-----
>> 4 files changed, 51 insertions(+), 30 deletions(-)
> 
> I tried this series, and it fixes the error log I reported.
> 
> On the other hand, I wonder if we could do something simpler.
> 
> The fd is only used by the registered handlers.
> If a handler reports that it does not want to watch this fd anymore,
> then there is no remaining user in the vhost library for this fd.
> 
> So my proposal would be to rename the "remove" flag as a "close" flag:
> 
> @@ -12,7 +12,7 @@ struct fdset;
> 
> #define MAX_FDS 1024
> 
> -typedef void (*fd_cb)(int fd, void *dat, int *remove);
> +typedef void (*fd_cb)(int fd, void *dat, int *close);
> 
> struct fdset *fdset_init(const char *name);
> 
> And defer closing to fd_man.
> Something like:
> 
> @@ -367,9 +367,9 @@ fdset_event_dispatch(void *arg)
>                        pthread_mutex_unlock(&pfdset->fd_mutex);
> 
>                        if (rcb && events[i].events & (EPOLLIN |
> EPOLLERR | EPOLLHUP))
> -                               rcb(fd, dat, &remove1);
> +                               rcb(fd, dat, &close1);
>                        if (wcb && events[i].events & (EPOLLOUT |
> EPOLLERR | EPOLLHUP))
> -                               wcb(fd, dat, &remove2);
> +                               wcb(fd, dat, &close2);
>                        pfdentry->busy = 0;
>                        /*
>                         * fdset_del needs to check busy flag.
> @@ -381,8 +381,10 @@ fdset_event_dispatch(void *arg)
>                         * fdentry not to be busy, so we can't call
>                         * fdset_del_locked().
>                         */
> -                       if (remove1 || remove2)
> +                       if (close1 || close2) {
>                                fdset_del(pfdset, fd);
> +                               close(fd);
> +                       }
>                }
> 
>                if (pfdset->destroy)
> 
> 
> And the only thing to move out of the socket and vduse handlers is the
> close(fd) call.
> 
> Like:
> 
> @@ -303,7 +303,7 @@ vhost_user_server_new_connection(int fd, void
> *dat, int *remove __rte_unused)
> }
> 
> static void
> -vhost_user_read_cb(int connfd, void *dat, int *remove)
> +vhost_user_read_cb(int connfd, void *dat, int *close)
> {
>        struct vhost_user_connection *conn = dat;
>        struct vhost_user_socket *vsocket = conn->vsocket;
> @@ -313,8 +313,7 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
>        if (ret < 0) {
>                struct virtio_net *dev = get_device(conn->vid);
> 
> -               close(connfd);
> -               *remove = 1;
> +               *close = 1;

I have one concern here is compared with this RFC, the proposal changed the timing
of close connfd，which means on QEMU side, cleaning up resources will happen later.

Currently I can’t think of issues could be introduced by this change (maybe you and
Maxime could remind me of something :)

Besides this, definitely this proposal is cleaner.

Thanks,
Chenbo 

> 
>                if (dev)
>                        vhost_destroy_device_notify(dev);
> 
> 
> Maxime, Chenbo, opinions?
> 
> 
> --
> David Marchand
> 

Re: [RFC 0/3] Vhost: fix FD entries cleanup

[Maxime Coquelin]

Hi Chenbo & David,

On 2/5/25 8:27 AM, Chenbo Xia wrote:
> Hi David,
> 
>> On Feb 4, 2025, at 21:18, David Marchand <david.marchand@redhat.com> wrote:
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hello vhost maintainers,
>>
>> On Tue, Dec 24, 2024 at 4:50 PM Maxime Coquelin
>> <maxime.coquelin@redhat.com> wrote:
>>>
>>> The vhost FD manager provides a way for the read/write
>>> callbacks to request removal of their associated FD from
>>> the epoll FD set. Problem is that it is missing a cleanup
>>> callback, so the read/write callback requesting the removal
>>> have to perform cleanups before the FD is removed from the
>>> FD set. It includes closing the FD before it is removed
>>> from the epoll FD set.
>>>
>>> This series introduces a new cleanup callback which, if
>>> implemented, is closed right after the FD is removed from
>>> FD set.
>>>
>>> Maxime Coquelin (3):
>>>   vhost: add cleanup callback to FD entries
>>>   vhost: fix vhost-user socket cleanup order
>>>   vhost: improve VDUSE reconnect handler cleanup
>>>
>>> lib/vhost/fd_man.c | 16 ++++++++++++----
>>> lib/vhost/fd_man.h |  3 ++-
>>> lib/vhost/socket.c | 46 ++++++++++++++++++++++++++--------------------
>>> lib/vhost/vduse.c  | 16 +++++++++++-----
>>> 4 files changed, 51 insertions(+), 30 deletions(-)
>>
>> I tried this series, and it fixes the error log I reported.
>>
>> On the other hand, I wonder if we could do something simpler.
>>
>> The fd is only used by the registered handlers.
>> If a handler reports that it does not want to watch this fd anymore,
>> then there is no remaining user in the vhost library for this fd.
>>
>> So my proposal would be to rename the "remove" flag as a "close" flag:
>>
>> @@ -12,7 +12,7 @@ struct fdset;
>>
>> #define MAX_FDS 1024
>>
>> -typedef void (*fd_cb)(int fd, void *dat, int *remove);
>> +typedef void (*fd_cb)(int fd, void *dat, int *close);
>>
>> struct fdset *fdset_init(const char *name);
>>
>> And defer closing to fd_man.
>> Something like:
>>
>> @@ -367,9 +367,9 @@ fdset_event_dispatch(void *arg)
>>                         pthread_mutex_unlock(&pfdset->fd_mutex);
>>
>>                         if (rcb && events[i].events & (EPOLLIN |
>> EPOLLERR | EPOLLHUP))
>> -                               rcb(fd, dat, &remove1);
>> +                               rcb(fd, dat, &close1);
>>                         if (wcb && events[i].events & (EPOLLOUT |
>> EPOLLERR | EPOLLHUP))
>> -                               wcb(fd, dat, &remove2);
>> +                               wcb(fd, dat, &close2);
>>                         pfdentry->busy = 0;
>>                         /*
>>                          * fdset_del needs to check busy flag.
>> @@ -381,8 +381,10 @@ fdset_event_dispatch(void *arg)
>>                          * fdentry not to be busy, so we can't call
>>                          * fdset_del_locked().
>>                          */
>> -                       if (remove1 || remove2)
>> +                       if (close1 || close2) {
>>                                 fdset_del(pfdset, fd);
>> +                               close(fd);
>> +                       }
>>                 }
>>
>>                 if (pfdset->destroy)
>>
>>
>> And the only thing to move out of the socket and vduse handlers is the
>> close(fd) call.
>>
>> Like:
>>
>> @@ -303,7 +303,7 @@ vhost_user_server_new_connection(int fd, void
>> *dat, int *remove __rte_unused)
>> }
>>
>> static void
>> -vhost_user_read_cb(int connfd, void *dat, int *remove)
>> +vhost_user_read_cb(int connfd, void *dat, int *close)
>> {
>>         struct vhost_user_connection *conn = dat;
>>         struct vhost_user_socket *vsocket = conn->vsocket;
>> @@ -313,8 +313,7 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
>>         if (ret < 0) {
>>                 struct virtio_net *dev = get_device(conn->vid);
>>
>> -               close(connfd);
>> -               *remove = 1;
>> +               *close = 1;
> 
> I have one concern here is compared with this RFC, the proposal changed the timing
> of close connfd，which means on QEMU side, cleaning up resources will happen later.
> 
> Currently I can’t think of issues could be introduced by this change (maybe you and
> Maxime could remind me of something :)

That's a good point.
I just tested David's suggestion with Vhost-user with OVS and QEMU:
- guest shutdown + reconnect
- live-migration
- OVS restart

It seems to behave very well.

> Besides this, definitely this proposal is cleaner.

I agree, I will send a new revision re-using David's proposal.

Thanks,
Maxime

> 
> Thanks,
> Chenbo
> 
>>
>>                 if (dev)
>>                         vhost_destroy_device_notify(dev);
>>
>>
>> Maxime, Chenbo, opinions?
>>
>>
>> --
>> David Marchand
>>
>