From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 21F1443C1D; Thu, 29 Feb 2024 14:32:14 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 09BA9402CD; Thu, 29 Feb 2024 14:32:14 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id B9691402B4 for ; Thu, 29 Feb 2024 14:32:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709213532; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uG0OCJFwN4GgUl84Tsf36ydHTidM6K0eu/ESIFnHp+E=; b=ANkAvB7aeT66I2YxXFZmGbIy3YIxvFI5L9R65tMDl6hPy5z5AdwE8NlXcGXd6y4rwTQs4F JpTC9ZS/UjWmjTX5lWv+N0fRtP9gyjxYFJvxQRSS1K/nlWPcP2mrcTZUj3kHuA6EZT0FQr bBdEkRtuSpUg2dWiSJhNbBS//Is9E8I= Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-377-wkR88SSWP5iHW1q1FaLV1A-1; Thu, 29 Feb 2024 08:32:11 -0500 X-MC-Unique: wkR88SSWP5iHW1q1FaLV1A-1 Received: by mail-lf1-f69.google.com with SMTP id 2adb3069b0e04-51325a4d003so820646e87.3 for ; Thu, 29 Feb 2024 05:32:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709213529; x=1709818329; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=uG0OCJFwN4GgUl84Tsf36ydHTidM6K0eu/ESIFnHp+E=; b=lY4OGffbRkZCR2RV4mVFJV0ZRc2mhoYSgAg43NmCC80LbU2Uzk5ybVemMa3v6F5no1 Ggq56lf+EsBwu7ujG0edN7c5TIeDIJs2H8LnmUlj3VrmPex5ijhb/qML4yDELudTh3Mh XR37KX44PN2/0jZhJTEZMdc65tUuBRnoz9mZ36JrLbJJr6txH846/BaRGaK7glo0zPYX VyA6KHq20BJtqtgM6Vo6xOzz6ZG7f2uCnOQOnDJ4QqRrQn44k0dv97T5O2psaXdbXJot psNAxv7QHfbeiBCcbRg6RMRBHV3qPxldF1kpQB4vU/M44m+aqnU4nAR8MO8+w2ZPmfS2 7Ftg== X-Gm-Message-State: AOJu0Ywdm2i/T8V8mA1Mu7Jr/S+joob2dawKFVmXf3xuARDM9gqFAaXr n9iIKlEnNVKdiKhI8aZiFdgfRbM8GA5CH7mS1pOLvYtZJEZWkfuHsTOc0Pg0EH+hO2228TP+nHD qDNoduLDePbeqSD2+9Ot8yOIqbKS/E3dGEbpD9sJ7Oo/aJ+MgVBfJRGUcMP+HsYEsCQyk7MEqyg kmN4+6YZ4jqqT15Kw= X-Received: by 2002:ac2:55a6:0:b0:513:28b9:76da with SMTP id y6-20020ac255a6000000b0051328b976damr959642lfg.46.1709213529455; Thu, 29 Feb 2024 05:32:09 -0800 (PST) X-Google-Smtp-Source: AGHT+IE3ZNDVD/Qex7xCO5C8zMmpayf5hGw75/IqNvbAseOD41ve5eMGFzJgy/rAn41+5LaE6pIi6Bw9RkqY86i2UD8= X-Received: by 2002:ac2:55a6:0:b0:513:28b9:76da with SMTP id y6-20020ac255a6000000b0051328b976damr959612lfg.46.1709213529073; Thu, 29 Feb 2024 05:32:09 -0800 (PST) MIME-Version: 1.0 References: <20240229122502.2572343-1-maxime.coquelin@redhat.com> <20240229122502.2572343-2-maxime.coquelin@redhat.com> In-Reply-To: <20240229122502.2572343-2-maxime.coquelin@redhat.com> From: David Marchand Date: Thu, 29 Feb 2024 14:31:57 +0100 Message-ID: Subject: Re: [PATCH 1/7] vhost: fix VDUSE device destruction failure To: Maxime Coquelin Cc: dev@dpdk.org, chenbox@nvidia.com, stable@dpdk.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hey Maxime, On Thu, Feb 29, 2024 at 1:25=E2=80=AFPM Maxime Coquelin wrote: > > 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() only returns once the pipe's read > callback has been executed. > > Fixes: 51d018fdac4e ("vhost: add VDUSE events handler") This looks to be a generic issue in the fd_man code. In practice, VDUSE only seems to be affected, so I am ok with this Fixes: t= ag. > Cc: stable@dpdk.org > > Signed-off-by: Maxime Coquelin > --- > lib/vhost/fd_man.c | 21 ++++++++++++++++++--- > lib/vhost/fd_man.h | 5 +++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c > index 79a8d2c006..42ce059039 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 =3D dat; > int r =3D 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_unuse= d, > * compiler happy > */ > RTE_SET_USED(r); > + > + pthread_mutex_lock(&fdset->sync_mutex); > + fdset->sync =3D 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 =3D 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, > @@ -359,7 +365,12 @@ fdset_pipe_init(struct fdset *fdset) > void > fdset_pipe_notify(struct fdset *fdset) > { > - int r =3D write(fdset->u.writefd, "1", 1); > + int r; > + > + pthread_mutex_lock(&fdset->sync_mutex); > + > + fdset->sync =3D false; > + r =3D write(fdset->u.writefd, "1", 1); > /* > * Just an optimization, we don't care if write() failed > * so ignore explicitly its return value to make the > @@ -367,4 +378,8 @@ fdset_pipe_notify(struct fdset *fdset) > */ > RTE_SET_USED(r); > > + 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..cc19937612 100644 > --- a/lib/vhost/fd_man.h > +++ b/lib/vhost/fd_man.h > @@ -6,6 +6,7 @@ > #define _FD_MAN_H_ > #include > #include > +#include > > #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; We should explicitly initialise those in https://git.dpdk.org/dpdk/tree/lib/vhost/socket.c#n91 and https://git.dpdk.org/dpdk/tree/lib/vhost/vduse.c#n34. The rest looks acceptable to me. --=20 David Marchand