Re: [PATCH] vhost: fix FD entries cleanup

[Chenbo Xia <chenbox@nvidia.com>]

> On Feb 6, 2025, at 16:41, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> With the recent rework of the FD manager to use epoll,
> an error message is emitted with Vhost-user at FD entry
> removal:
> 
> ERR|VHOST_FDMAN: could not remove 102 fd from 101 epfd: No such file or directory
> 
> It occurs because the read callback closes the file
> descriptor before it is removed from the FD set.
> This patch defers the close() after the FD entry is removed
> from the set.
> 
> Fixes: 0e38b42bf61c ("vhost: manage FD with epoll")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/fd_man.c | 12 +++++++-----
> lib/vhost/fd_man.h |  2 +-
> lib/vhost/socket.c | 11 +++++------
> lib/vhost/vduse.c  |  9 ++++-----
> 4 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
> index 9bc7e50b93..f9147edee7 100644
> --- a/lib/vhost/fd_man.c
> +++ b/lib/vhost/fd_man.c
> @@ -333,7 +333,7 @@ fdset_event_dispatch(void *arg)
>        fd_cb rcb, wcb;
>        void *dat;
>        int fd, numfds;
> -       int remove1, remove2;
> +       int close1, close2;
>        struct fdset *pfdset = arg;
> 
>        if (pfdset == NULL)
> @@ -357,7 +357,7 @@ fdset_event_dispatch(void *arg)
>                                continue;
>                        }
> 
> -                       remove1 = remove2 = 0;
> +                       close1 = close2 = 0;
> 
>                        rcb = pfdentry->rcb;
>                        wcb = pfdentry->wcb;
> @@ -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)
> diff --git a/lib/vhost/fd_man.h b/lib/vhost/fd_man.h
> index 6398343a6a..eadcc6fb42 100644
> --- a/lib/vhost/fd_man.h
> +++ b/lib/vhost/fd_man.h
> @@ -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);
> 
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index 433a42bf80..b27020ae8d 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -84,8 +84,8 @@ struct vhost_user {
> 
> #define MAX_VIRTIO_BACKLOG 128
> 
> -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_server_new_connection(int fd, void *data, int *close);
> +static void vhost_user_read_cb(int fd, void *dat, int *close);
> static int create_unix_socket(struct vhost_user_socket *vsocket);
> static int vhost_user_start_client(struct vhost_user_socket *vsocket);
> 
> @@ -290,7 +290,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
> 
> /* call back when there is new vhost-user connection from client  */
> static void
> -vhost_user_server_new_connection(int fd, void *dat, int *remove __rte_unused)
> +vhost_user_server_new_connection(int fd, void *dat, int *close __rte_unused)
> {
>        struct vhost_user_socket *vsocket = dat;
> 
> @@ -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);
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index 8ba58555f9..9d39e71c68 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -117,7 +117,7 @@ static struct vhost_backend_ops vduse_backend_ops = {
> };
> 
> static void
> -vduse_control_queue_event(int fd, void *arg, int *remove __rte_unused)
> +vduse_control_queue_event(int fd, void *arg, int *close __rte_unused)
> {
>        struct virtio_net *dev = arg;
>        uint64_t buf;
> @@ -350,7 +350,7 @@ vduse_device_stop(struct virtio_net *dev)
> }
> 
> static void
> -vduse_events_handler(int fd, void *arg, int *remove __rte_unused)
> +vduse_events_handler(int fd, void *arg, int *close __rte_unused)
> {
>        struct virtio_net *dev = arg;
>        struct vduse_dev_request req;
> @@ -563,14 +563,13 @@ 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 *close)
> {
>        struct virtio_net *dev = arg;
> 
>        vduse_device_start(dev, true);
> 
> -       close(fd);
> -       *remove = 1;
> +       *close = 1;
> }
> 
> static int
> --
> 2.48.1
> 

Reviewed-by: Chenbo Xia <chenbox@nvidia.com>