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 51D9543C1D for ; Thu, 29 Feb 2024 14:32:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 432E242D80; Thu, 29 Feb 2024 14:32:15 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 1B0C7402CD for ; Thu, 29 Feb 2024 14:32:13 +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-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-452-FzV3cmbHP2KEmJZG6Rov4Q-1; Thu, 29 Feb 2024 08:32:11 -0500 X-MC-Unique: FzV3cmbHP2KEmJZG6Rov4Q-1 Received: by mail-lf1-f72.google.com with SMTP id 2adb3069b0e04-51313b50f1bso539759e87.0 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=Wl3PeFTPl8xMaweE+M5EHMniGFY7p5cvKnknGPAqqaJhdDf+XqK8XfVAWvii2EOChK bdOCM/271dE35DCtPn7SRBLUlphRy6A4/9CMCIUhHYEQSWAUhAmvZySeTD6t3S34WIky hTabYjZNeavlpdT4CTm1TVI4K1W8kPlgk+2cU6LXTHZoCL83UqjRjhBNdTFjUbQ0FlB8 3hG9KBuKIHf0l3jrAMSoZw6M8n76khZqlXKNj8uKJvH4SNcEuRZm30PtfJ1azKJgOhAU CkRpxV+atrfzvwnl+j73QBhR/tuHbReZw3PisonXLLRJf/W5cP3cvW4vEKIu1mFfmVvN r7vQ== X-Forwarded-Encrypted: i=1; AJvYcCWtkSCxbGyH0YUwfudRMC8MipoplNsdJ+yKKZaBd+OSw62jTuqwjj8iBo776HKYDbLGomh/MlLD1JUZ9tDTafM= X-Gm-Message-State: AOJu0YyhHzSrYqipcb5HjlSZrgnf9/53rumNtu9CzG3wIWKQCwRsluLD cAEN/GdwH33NRRUDrVz2BYSAHSRNqHGtf8Xwf1MvwK2BlbQhK6LDXT1K47EiHhKyhcjBC8Evu50 VT1DUuI2q/P12i6u06S/Jpj3YugDyW3ICwKzHqWUBt50ZNeNxEyipdGrBG0KApkXR4olhBy5Pyq Mey4LOdbswQ0FaTd4Dma0= X-Received: by 2002:ac2:55a6:0:b0:513:28b9:76da with SMTP id y6-20020ac255a6000000b0051328b976damr959641lfg.46.1709213529453; 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: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-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