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 A75E9A0C47; Thu, 14 Oct 2021 14:12:59 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3D0694112E; Thu, 14 Oct 2021 14:12:59 +0200 (CEST) Received: from mail-ua1-f47.google.com (mail-ua1-f47.google.com [209.85.222.47]) by mails.dpdk.org (Postfix) with ESMTP id 5AE5A40041 for ; Thu, 14 Oct 2021 14:12:58 +0200 (CEST) Received: by mail-ua1-f47.google.com with SMTP id e2so10775086uax.7 for ; Thu, 14 Oct 2021 05:12:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=smartx-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vvZttWcrDVgPc8YYSjrJfQkL/FTHLwr/p7O1qbrX3xY=; b=xPqXNQ28V3A2k1e4xmqQeaOn9cVURqR424EQG/MVtyhCiR6ngdZhW/mNMUje7HGGkP xFE7aGWnhhY5II8D8Az6V5RmaHAUaia+D3e3UJZ0FAHhgDvyC3hui11cntmy8Sz2/T4e 0pdqOs1hJ3c57EyGXdIBAKpQbx9coMdZMtGbOrYiP76yRIOg1QRN4uLbhihMQC1SG5Dr xSf9fiuawjdjMGnaQN37l0hJihT0PplInUaCpTCoWL2aCbHUZyqMoTY5p17AlYPb+GgO H6tDg890mbpJGzENE94GO5bUimh2ebkn7LSZND3zTmB0qrLmwoXIIA2s6sW6P4uk+YXE yNDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vvZttWcrDVgPc8YYSjrJfQkL/FTHLwr/p7O1qbrX3xY=; b=MMzknVTaa79UOoNeaJfU0HOhKWGjlxp2At48N5AP9dV6i7F9ohjx7sU2b2JKUufkBC rv2pD8PG8NDcUc2cANbd/00Nkz0i221D7DEN2F789hpcQborHsB6LFbRH9Ij1oZGkXEb HSZHfz3JQ1tixPEmsEdIV6ZGWc+rZUZPtQ/bW57qVs2sSD1+hWTN+gvR4rdVe953Cvv5 hDzGhsBZf6KcvANeuUJ5EQQMzGFrzAP/Yy8P1sZIW1HQM1NMUbLFcdKrZOFpzoyVZo+8 1XR+SkWEs0BCueuWouHoo726OPmgKF/kyTkXNln+kS83WtVy5SbCo84klS0mj/9TLI3O DhqA== X-Gm-Message-State: AOAM5339X7uSJ9NWIZeEyXSRWhLdxhk0WCUjTQaRaDqeQVzhtBXh9EE9 VHoQ8grv9Dq0htHdJZgX74IQIgCLNJYrDE7CRyev8Q== X-Google-Smtp-Source: ABdhPJzW81OkmTOsfy8JVMfEnm/2JtYYuuqywY+VhQq7ZkiWEfcZyNcQzhNN4hUbUlLfn96a3LHOvD6zwpSNXVCi8IM= X-Received: by 2002:a67:fdd3:: with SMTP id l19mr6132636vsq.37.1634213577636; Thu, 14 Oct 2021 05:12:57 -0700 (PDT) MIME-Version: 1.0 References: <20210903075637.2201185-1-fengli@smartx.com> <20210903080203.2495559-1-fengli@smartx.com> <34b02edd-b44c-0f75-a1ff-5c78803783cf@redhat.com> In-Reply-To: <34b02edd-b44c-0f75-a1ff-5c78803783cf@redhat.com> From: Li Feng Date: Thu, 14 Oct 2021 20:12:46 +0800 Message-ID: To: Maxime Coquelin Cc: Chenbo Xia , dev Content-Type: text/plain; charset="UTF-8" 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 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)) -- 2.1.0 Do you have any better ideas about this issue? Thanks. > > Regards, > Maxime > > > } > > > > /* > > >