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 3FF79A0C47; Thu, 14 Oct 2021 14:28:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2976C40E50; Thu, 14 Oct 2021 14:28: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 A0A5440041 for ; Thu, 14 Oct 2021 14:28:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634214514; 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=beIE4s+uCLKDnkWFEV1nAw6lvC0WkWQ87c/6VaNxJEg=; b=W8gs+p/GMz3r4PltLbXRRBda6NiLduKuhs+TkXBtXPrJ9nrCECwJG8RACrgSMwoM0O7n88 tY8rpxA4xhNqOMyC8qz2UHPXD5QN5Wq9QuOB5a8bgoPbggoSzemCL0s5hPegUFVvRa38YT oxDSOwcW2/EWOqQtdLM6a+IsIDgjRt0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-118-osT5KHRXMme6Zyz0BTuPWQ-1; Thu, 14 Oct 2021 08:28:30 -0400 X-MC-Unique: osT5KHRXMme6Zyz0BTuPWQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D89931835ACC; Thu, 14 Oct 2021 12:28:29 +0000 (UTC) Received: from [10.39.208.20] (unknown [10.39.208.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E3AD610016F5; Thu, 14 Oct 2021 12:28:28 +0000 (UTC) Message-ID: Date: Thu, 14 Oct 2021 14:28:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 To: Li Feng Cc: Chenbo Xia , dev References: <20210903075637.2201185-1-fengli@smartx.com> <20210903080203.2495559-1-fengli@smartx.com> <34b02edd-b44c-0f75-a1ff-5c78803783cf@redhat.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com 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 Subject: Re: [dpdk-dev] [PATCH v2] vhost: call destroy_device always in vhost_destroy_device_notify 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 Sender: "dev" On 10/14/21 14:12, Li Feng wrote: > On Thu, Oct 14, 2021 at 8:01 PM Maxime Coquelin > wrote: >> >> Hi Li, >> >> The commit message is not compliant with the contributors guidelines: >> https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-subject-line > OK, I got it. >> >> On 9/3/21 10:02, Li Feng wrote: >>> Vhost-user client must send the mem table, kick fd, call fd on all >>> virtqueues, then the device will be VIRTIO_DEV_RUNNING. >>> >>> If the vhost-user communication is initialized partly, e.g. >>> - When initializing the vhost-user, try to restart the vhost-user >>> backend; >>> - Seabios only initialized the vhost-scsi req vq. >>> The device is not with flags VIRTIO_DEV_RUNNING.. >>> >>> Root Cause: >>> The vhost session has been created, and added the scsi/blk requestq >>> poller into reactor, but when destroying the device, the requestq is not >>> unregistered. >>> >>> Reproduce the crash on spdk vhost-user backend: >>> 1. Create a VM; >>> 2. Mount a ISO to a VM, start the VM, don't install the OS; >>> 3. Restart the spdk_tgt; >>> >>> Another discusstion is in seabiso: >>> https://patchew.org/Seabios/20210831122339.2591585-1-fengli@smartx.com/ >> >> This is a fix, so you need to add the Fixes tag and cc stable. > Acked. > >> >>> Signed-off-by: Li Feng >>> --- >>> v2: >>> Fix the commit msg typo: vas -> virtqueues. >>> -- >>> lib/vhost/vhost.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c >>> index 355ff37651..191ba82c41 100644 >>> --- a/lib/vhost/vhost.c >>> +++ b/lib/vhost/vhost.c >>> @@ -710,8 +710,8 @@ vhost_destroy_device_notify(struct virtio_net *dev) >>> if (vdpa_dev) >>> vdpa_dev->ops->dev_close(dev->vid); >>> dev->flags &= ~VIRTIO_DEV_RUNNING; >>> - dev->notify_ops->destroy_device(dev->vid); >>> } >>> + dev->notify_ops->destroy_device(dev->vid); >> >> .destroy_device() is the counter-part of .new_device(). >> VIRTIO_DEV_RUNNING is set only when .new_device() has been called with >> success, and cleared when .destroy_device() is called. >> >> So I disagree with the fix, we want to keep the correlation between >> VIRTIO_DEV_RUNNING and .new_device()/.destroy_device(). Doing otherwise >> could lead to regressions with other applications than yours. >> >> What is not clear from the commit message or the discussion you link is >> where does it crash exactly. Is it in SPDK, in DPDK? > > The crash is in SPDK, the poller is still running in the reactor, > however, the device is freed. > > I really don't have a good method to handle this partly initialized virt queues. > This is another patch I prepared to fix this issue: > > From 63142ec60088d08b27b9657640b82e837557b5d4 Mon Sep 17 00:00:00 2001 > From: Li Feng > Date: Wed, 1 Sep 2021 16:51:44 +0800 > Subject: [PATCH] vhost: fix vhost session crash > > If any vq is inited well, treat the dev is RUNNING status. > > Root Cause: > The session has been created, and added the requestq poller into > reactor, but when destroying the device, the requestq is not > unregistered. > The seabios only initialized the req vq(idx = 2), ignore the controlq > and eventq vq. > > Reproduce: > 1. Create a VM; > 2. Mount a ISO to a VM, start the VM, don't install the OS; > 3. Restart the zbs-chunkd; > > Signed-off-by: Li Feng > Change-Id: I21292e58b0b08237b5d105359095ec6a31907752 > --- > lib/librte_vhost/vhost_user.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index f211ec8..a80e9f4 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -1394,9 +1394,11 @@ virtio_is_ready(struct virtio_net *dev) > "kickfd: %d callfd: %d enabled: %d\n", > dev->ifname, vq, i, vq->desc, vq->avail, > vq->used, vq->kickfd, vq->callfd, vq->enabled); > - if (!vq_is_ready(dev, vq)) > - return 0; > + if (vq_is_ready(dev, vq)) > + break; > } > + if (i == nr_vring) > + return 0; > > /* If supported, ensure the frontend is really done with config */ > if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS)) > Above patch will also cause regression, as networking backends work with queue pairs. So your issue is that SPDK is processing vrings while DPDK considers the device as not running. Instead of working around that issue, maybe what you should do is to introduce a new API and mechanism to help DPDK determine whether it should consider the device ready based on the backend type. IIUC, in your Vhost-scsi case, it should be as soon as VQ 2 is ready?