From: Maxime Coquelin <mcoqueli@redhat.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev <dev@dpdk.org>, Maxime Coquelin <maxime.coquelin@redhat.com>,
stable@dpdk.org, Chenbo Xia <chenbox@nvidia.com>
Subject: Re: [PATCH v2] vhost: fix VDUSE device destruction failure
Date: Mon, 4 Mar 2024 16:12:57 +0100 [thread overview]
Message-ID: <CAO55cswDhtT8SbP_oA_ZqE1nbdfmUztO+jVsPaB56i6WM+B-Pg@mail.gmail.com> (raw)
In-Reply-To: <20240304103558.1500695-1-david.marchand@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 5353 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 7221 bytes --]
next prev parent reply other threads:[~2024-03-04 15:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240229122502.2572343-1-maxime.coquelin@redhat.com>
2024-02-29 12:24 ` [PATCH 1/7] " Maxime Coquelin
2024-02-29 13:31 ` David Marchand
2024-03-04 10:35 ` [PATCH v2] " David Marchand
2024-03-04 15:12 ` Maxime Coquelin [this message]
2024-03-05 9:05 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAO55cswDhtT8SbP_oA_ZqE1nbdfmUztO+jVsPaB56i6WM+B-Pg@mail.gmail.com \
--to=mcoqueli@redhat.com \
--cc=chenbox@nvidia.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).