Le lun. 4 mars 2024, 11:36, David Marchand <david.marchand@redhat.com> a
écrit :

> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> VDUSE_DESTROY_DEVICE ioctl can fail because the device's
> chardev is not released despite close syscall having been
> called. It happens because the events handler thread is
> still polling the file descriptor.
>
> fdset_pipe_notify() is not enough because it does not
> ensure the notification has been handled by the event
> thread, it just returns once the notification is sent.
>
> To fix this, this patch introduces a synchronization
> mechanism based on pthread's condition, so that
> fdset_pipe_notify_sync() only returns once the pipe's
> read callback has been executed.
>
> Fixes: 51d018fdac4e ("vhost: add VDUSE events handler")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - sync'd only when in VDUSE destruction path,
> - added explicit init of sync_mutex,
>
> ---
>  lib/vhost/fd_man.c | 23 +++++++++++++++++++++--
>  lib/vhost/fd_man.h |  6 ++++++
>  lib/vhost/socket.c |  1 +
>  lib/vhost/vduse.c  |  3 ++-
>  4 files changed, 30 insertions(+), 3 deletions(-)
>

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks for improving the patch,
Maxime


> diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
> index 79a8d2c006..481e6b900a 100644
> --- a/lib/vhost/fd_man.c
> +++ b/lib/vhost/fd_man.c
> @@ -309,10 +309,11 @@ fdset_event_dispatch(void *arg)
>  }
>
>  static void
> -fdset_pipe_read_cb(int readfd, void *dat __rte_unused,
> +fdset_pipe_read_cb(int readfd, void *dat,
>                    int *remove __rte_unused)
>  {
>         char charbuf[16];
> +       struct fdset *fdset = dat;
>         int r = read(readfd, charbuf, sizeof(charbuf));
>         /*
>          * Just an optimization, we don't care if read() failed
> @@ -320,6 +321,11 @@ fdset_pipe_read_cb(int readfd, void *dat __rte_unused,
>          * compiler happy
>          */
>         RTE_SET_USED(r);
> +
> +       pthread_mutex_lock(&fdset->sync_mutex);
> +       fdset->sync = true;
> +       pthread_cond_broadcast(&fdset->sync_cond);
> +       pthread_mutex_unlock(&fdset->sync_mutex);
>  }
>
>  void
> @@ -342,7 +348,7 @@ fdset_pipe_init(struct fdset *fdset)
>         }
>
>         ret = fdset_add(fdset, fdset->u.readfd,
> -                       fdset_pipe_read_cb, NULL, NULL);
> +                       fdset_pipe_read_cb, NULL, fdset);
>
>         if (ret < 0) {
>                 VHOST_FDMAN_LOG(ERR,
> @@ -366,5 +372,18 @@ fdset_pipe_notify(struct fdset *fdset)
>          * compiler happy
>          */
>         RTE_SET_USED(r);
> +}
> +
> +void
> +fdset_pipe_notify_sync(struct fdset *fdset)
> +{
> +       pthread_mutex_lock(&fdset->sync_mutex);
> +
> +       fdset->sync = false;
> +       fdset_pipe_notify(fdset);
> +
> +       while (!fdset->sync)
> +               pthread_cond_wait(&fdset->sync_cond, &fdset->sync_mutex);
>
> +       pthread_mutex_unlock(&fdset->sync_mutex);
>  }
> diff --git a/lib/vhost/fd_man.h b/lib/vhost/fd_man.h
> index 6315904c8e..7816fb11ac 100644
> --- a/lib/vhost/fd_man.h
> +++ b/lib/vhost/fd_man.h
> @@ -6,6 +6,7 @@
>  #define _FD_MAN_H_
>  #include <pthread.h>
>  #include <poll.h>
> +#include <stdbool.h>
>
>  #define MAX_FDS 1024
>
> @@ -35,6 +36,10 @@ struct fdset {
>                         int writefd;
>                 };
>         } u;
> +
> +       pthread_mutex_t sync_mutex;
> +       pthread_cond_t sync_cond;
> +       bool sync;
>  };
>
>
> @@ -53,5 +58,6 @@ int fdset_pipe_init(struct fdset *fdset);
>  void fdset_pipe_uninit(struct fdset *fdset);
>
>  void fdset_pipe_notify(struct fdset *fdset);
> +void fdset_pipe_notify_sync(struct fdset *fdset);
>
>  #endif
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index a2fdac30a4..96b3ab5595 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -93,6 +93,7 @@ static struct vhost_user vhost_user = {
>                 .fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
>                 .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>                 .fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
> +               .sync_mutex = PTHREAD_MUTEX_INITIALIZER,
>                 .num = 0
>         },
>         .vsocket_cnt = 0,
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index d462428d2c..e0c6991b69 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -36,6 +36,7 @@ static struct vduse vduse = {
>                 .fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
>                 .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>                 .fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
> +               .sync_mutex = PTHREAD_MUTEX_INITIALIZER,
>                 .num = 0
>         },
>  };
> @@ -618,7 +619,7 @@ vduse_device_destroy(const char *path)
>         vduse_device_stop(dev);
>
>         fdset_del(&vduse.fdset, dev->vduse_dev_fd);
> -       fdset_pipe_notify(&vduse.fdset);
> +       fdset_pipe_notify_sync(&vduse.fdset);
>
>         if (dev->vduse_dev_fd >= 0) {
>                 close(dev->vduse_dev_fd);
> --
> 2.43.0
>
>