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 C41B6A0548; Tue, 11 Oct 2022 18:44:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6CA5F40F19; Tue, 11 Oct 2022 18:44:36 +0200 (CEST) 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 4109B40E50 for ; Tue, 11 Oct 2022 18:44:35 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1665506674; 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=ISyObjqT8cpwxDXhUFDicKNLDo5TCjAr/aM46tV4nMc=; b=gO9qCyyvq5hsl3lGdmMlyhYfS5+brOZipdDn9jN0Acua6vaIxeyI141jsjzrIAYg4v7x/c aEhgd3yHkjoXucbYaV27WfXd9HmuqFhp7fg73l+5HKMWOS2zmTikprdvnAOal2nDzybqY0 RVCtA1ob/ZUviAI4hF84yXacEs/Ucd8= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-218-Oj135_rlOvSAGVn845IWyw-1; Tue, 11 Oct 2022 12:44:31 -0400 X-MC-Unique: Oj135_rlOvSAGVn845IWyw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4A25F1C05AE6; Tue, 11 Oct 2022 16:44:31 +0000 (UTC) Received: from [10.39.208.19] (unknown [10.39.208.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D8054404CD98; Tue, 11 Oct 2022 16:44:29 +0000 (UTC) Message-ID: <72b778f6-c813-4b20-1b9c-834d22191a5b@redhat.com> Date: Tue, 11 Oct 2022 18:44:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 To: Kangjie Xu , chenbo.xia@intel.com Cc: dev@dpdk.org, xuanzhuo@linux.alibaba.com, hengqi@linux.alibaba.com, jasowang@redhat.com, mst@redhat.com References: <0383bb821dd65d8511a91e9f13b193230be59557.1662952732.git.kangjie.xu@linux.alibaba.com> From: Maxime Coquelin Subject: Re: [PATCH v3 1/2] vhost: destroy device when all vqs are inactive In-Reply-To: <0383bb821dd65d8511a91e9f13b193230be59557.1662952732.git.kangjie.xu@linux.alibaba.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 On 9/12/22 05:36, Kangjie Xu wrote: > We change the behavior of vhost_user_get_vring_base(). Previosly, > destroying a virtqueue will cause the whole device to be destroyed. > The behavior is not specified in the vhost-user protocol. > > Thus, we refactor this part. The device will be destroyed only when > all virtqueues in the device are going to be destroyed. > > This helps us to simplify the implementation when resetting a virtqueue. > > Signed-off-by: Kangjie Xu > Signed-off-by: Xuan Zhuo > --- > lib/vhost/vhost_user.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c > index 4ad28bac45..a9f0709f94 100644 > --- a/lib/vhost/vhost_user.c > +++ b/lib/vhost/vhost_user.c > @@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev, > { > struct virtio_net *dev = *pdev; > struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index]; > + uint32_t i, num_live_vring = 0; > uint64_t val; > > - /* We have to stop the queue (virtio) if it is running. */ > - vhost_destroy_device_notify(dev); > + /* Stop the device when vq is the last active queue */ > + for (i = 0; i < dev->nr_vring; i++) > + if (dev->virtqueue[i]->access_ok) > + num_live_vring++; > + > + if (num_live_vring == 1 && vq->access_ok) > + vhost_destroy_device_notify(dev); > > dev->flags &= ~VIRTIO_DEV_READY; > dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED; I think we are missing something here. We used to send the device destroy notification before getting the ring indexes, in order to ensure that the application has stopped processing the rings. With this patch, the application may still be polling the ring while we get the ring indexes (e.g. a thread in the application may be in the middle of rte_vhost_dequeue_burst() on that ring). So at best the ring indexes returned to the Vhost-user master will be outdated. At worst, it will crash the application because we call vring_invalidate() without the vq's lock being taken. I think you should protect all the VQ indexes fetching and VQ deinit using its access_lock. Maxime