vhost: fix FD entries cleanup

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(-)
  

On Thu, Feb 6, 2025 at 9:42 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> 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>

LGTM.
  

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

On 2/6/25 9:41 AM, Maxime Coquelin wrote:
> 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(-)
> 

Applied to next-virtio/for-next-net.

Thanks,
Maxime