From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f54.google.com (mail-pa0-f54.google.com [209.85.220.54]) by dpdk.org (Postfix) with ESMTP id 892428DAA for ; Tue, 27 Oct 2015 10:25:16 +0100 (CET) Received: by padhk11 with SMTP id hk11so217531111pad.1 for ; Tue, 27 Oct 2015 02:25:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=igel_co_jp.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type:content-transfer-encoding; bh=VnCsThfYwFia1Ob6cKoy41aYbyriCCtF3/eg5wfrbE4=; b=DADW5Va1RoGwiE4WiIRanDoDwiZ2LiVKAInXblhw+XhZxkhrB9SrtRWQ02ZrNwcLf7 f4oVzcQRlxoo7uHScYXBZswLx9XI7LFwyz8NCQ0OYVL1MuCURR0qJFO5LeLZO2T9VjoO kGeu/zsKbBhwPPf9Xm4E6c8C1yrrktQsAsNZiddQDojKnn+MtTvWJW3OSnA7Qxz6wnyN 5QwI1gRvOfZXWvWj9JN65UnxsaJ0sx+hgV2Ryo/iwY67Az8FM6dwIawZQJDM3pBf7i0n viHTt52OOwJhT+cAx9JLRT+bsQXxWsEFtDGqcBfoSygccI0xJCrVE0xyWBzaJH7w479E 1R0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=VnCsThfYwFia1Ob6cKoy41aYbyriCCtF3/eg5wfrbE4=; b=ag0pN0k5znmPEa8cQegehBOEStIzxQW69580om9qlXaoyesPUA5EOYViCwRJivam4o HoNgugwUfYYlOmsnF4+VMi8hVAJ1WNP5wuVz+QCVzvQt96+hGL7KpLdKR0diLam0Aaee 4SQK/V1ZCA+vRyOOfTR8divCJpd36f4WUPtQX5JYmz7xfSxC2GM28yI1SKFZLZxFkkj8 E0nOFsjreMsd3aierXi6zaZ2gRAUV5YSRFZxG0HFiRVU93GwtWURifS2FGLQlRbAh1PE EQ3jMza7b6Cl1V69MPWhv1y88zY/LD89ElwYNQleP2U7I2pdIR6hJclDkAbTPNQTGaxN vd+A== X-Gm-Message-State: ALoCoQkSVpjq+EXsUnbbF84C4IyNRZA3ka3+TPMsICVvZ9t0FmD+Pm37u3uJs7hVANYbSBuvDafR X-Received: by 10.66.124.200 with SMTP id mk8mr26151899pab.25.1445937915758; Tue, 27 Oct 2015 02:25:15 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by smtp.googlemail.com with ESMTPSA id lo9sm38582806pab.19.2015.10.27.02.25.13 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Oct 2015 02:25:15 -0700 (PDT) To: Yuanhan Liu , "Xie, Huawei" References: <1445932306-11880-1-git-send-email-mukawa@igel.co.jp> <20151027083957.GG3115@yliu-dev.sh.intel.com> From: Tetsuya Mukawa X-Enigmail-Draft-Status: N1110 Message-ID: <562F42F9.8050603@igel.co.jp> Date: Tue, 27 Oct 2015 18:25:13 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151027083957.GG3115@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" , "ann.zhuangyanying@huawei.com" Subject: Re: [dpdk-dev] [PATCH] vhost: Fix wrong handling of virtqueue array index X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Oct 2015 09:25:17 -0000 On 2015/10/27 17:39, Yuanhan Liu wrote: > On Tue, Oct 27, 2015 at 08:24:00AM +0000, Xie, Huawei wrote: >> On 10/27/2015 3:52 PM, Tetsuya Mukawa wrote: >>> The patch fixes wrong handling of virtqueue array index when >>> GET_VRING_BASE message comes. >>> The vhost backend will receive the message per virtqueue. >>> Also we should call a destroy callback handler when both RXQ >>> and TXQ receives the message. >>> >>> Signed-off-by: Tetsuya Mukawa >>> --- >>> lib/librte_vhost/vhost_user/virtio-net-user.c | 20 ++++++++++---------- >>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c >>> index a998ad8..99c075f 100644 >>> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c >>> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c >>> @@ -283,12 +283,10 @@ user_get_vring_base(struct vhost_device_ctx ctx, >>> struct vhost_vring_state *state) >>> { >>> struct virtio_net *dev = get_device(ctx); >>> + uint16_t base_idx = state->index / VIRTIO_QNUM * VIRTIO_QNUM; >>> >>> if (dev == NULL) >>> return -1; >>> - /* We have to stop the queue (virtio) if it is running. */ >>> - if (dev->flags & VIRTIO_DEV_RUNNING) >>> - notify_ops->destroy_device(dev); >> Hi Tetsuya: >> I don't understand why we move it to the end of the function. >> If we don't tell the application to remove the virtio device from the > As you stated, he just moved it to the end of the function: it > still does invoke notfiy_ops->destroy_device() in the end. > > And the reason he moved it to the end is he want to invoke the > callback just when the second GET_VRING_BASE message is received > for the queue pair. And while thinking twice, it's not necessary, > as we will do the "flags & VIRTIO_DEV_RUNNING" check first, it > doesn't matter on which virt queue we invoke the callback. I thought we had 2 choices. 1. Call the callback handler at first place of this function when 1st GET_VRING_BASE message comes. 2. Call the callback handler at last place of this function when 2nd GET_VRING_BASE message comes. And I chose 2nd, because in the case of 1st choice, before sending 2nd message, QEMU guess one of queue is still alive, but actually in DPDK application, it has been closed already. I thought above inconsistency might cause the issue. But yes, if we chose 2nd, we may have an issue as Xie said. > > --yliu > >> data plane, then the vhost application is still operating on that >> device, we shouldn't do anything to the virtio_net device. >> For this case, as vhost doesn't use kickfd, it will not cause issue, but >> i think it is best practice firstly to remove it from data plan through >> destroy_device. >> >> I think we could call destroy_device the first time we receive this >> message. Currently we don't have per queue granularity control to only >> remove one queue from data plane. >> >> I am Okay to only close the kickfd for the specified queue index. >> >> Btw, do you meet issue with previous implementation? Yes, I faced illegal memory access. For example, if we have RX and TX queues, we will have 2 GET_VRING_BASE messages when virtio-net device is finalized. While handling these messages, 'dev->virtqueue[2]' will be accessed, then will cause illegal access. (We only have 2 queues, so above will be NULL) So actually we need to change the function a bit. Thanks, Tetsuya