From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <maxime.coquelin@redhat.com>
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
by dpdk.org (Postfix) with ESMTP id 1367612009
for <dev@dpdk.org>; Thu, 1 Feb 2018 13:49:13 +0100 (CET)
Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com
[10.5.11.14])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by mx1.redhat.com (Postfix) with ESMTPS id 6B7592D0FAA;
Thu, 1 Feb 2018 12:49:12 +0000 (UTC)
Received: from [10.36.112.31] (ovpn-112-31.ams2.redhat.com [10.36.112.31])
by smtp.corp.redhat.com (Postfix) with ESMTPS id C33F15D96E;
Thu, 1 Feb 2018 12:49:07 +0000 (UTC)
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: dev@dpdk.org, Yuanhan Liu <yliu@fridaylinux.org>
References: <20180131174651.6386-1-stefanha@redhat.com>
<20180131174651.6386-3-stefanha@redhat.com>
<0032747e-c981-ebd8-357b-b24122c89993@redhat.com>
<20180201102428.GA5783@stefanha-x1.localdomain>
From: Maxime Coquelin <maxime.coquelin@redhat.com>
Message-ID: <cfb9d2fd-c56d-4d15-4d84-2b9595aee655@redhat.com>
Date: Thu, 1 Feb 2018 13:49:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20180201102428.GA5783@stefanha-x1.localdomain>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16
(mx1.redhat.com [10.5.110.29]); Thu, 01 Feb 2018 12:49:12 +0000 (UTC)
Subject: Re: [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with built-in
virtio_net.c driver
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 01 Feb 2018 12:49:13 -0000
On 02/01/2018 11:24 AM, Stefan Hajnoczi wrote:
> On Wed, Jan 31, 2018 at 07:07:50PM +0100, Maxime Coquelin wrote:
>> Hi Stefan,
>>
>> On 01/31/2018 06:46 PM, Stefan Hajnoczi wrote:
>>> Commit e29109323595beb3884da58126ebb3b878cb66f5 ("vhost: destroy unused
>>> virtqueues when multiqueue not negotiated") broke vhost-scsi by removing
>>> virtqueues when the virtio-net-specific VIRTIO_NET_F_MQ feature bit is
>>> missing.
>>>
>>> The vhost_user.c code shouldn't assume all devices are vhost net device
>>> backends. Use the new VIRTIO_DEV_BUILTIN_VIRTIO_NET flag to check
>>> whether virtio_net.c is being used.
>>>
>>> This fixes examples/vhost_scsi.
>>>
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 1dd1a61b6..65ee33919 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -187,7 +187,8 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>>> (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>>> (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>>> - if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
>>> + if ((dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) &&
>>> + !(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
>>
>> If we had an external net backend using the library, we would also need to
>> remove extra queues if it advertised VIRTIO_NET_F_MQ, but the feature
>> isn't negotiated.
>
> A net-specific fix inside librte_vhost is not enough since non-net
> drivers may only initialize a subset of the virtqueues.
You are right, and even with net backend, the driver may decide to only
initialize a subset of the virtqueues even if VIRTIO_NET_F_MQ is
negotiated.
This is not the case today with virtio-net Linux kernel driver and
DPDK's Virtio PMD, but Windows virtio-net driver only initialize as much
queue pairs as online vCPUs.
> I suggest starting over. Get rid of the net-specific fix and instead
> look at when new_device() needs to be called.
I agree, and already started to work on it. But my understanding is that
we will need a vhost-user protocol update. So I implemented this
workaround to support the VIRTIO_NET_F_MQ not negotiated case, which
happens with iPXE.
> Virtqueues will not be changed after the DRIVER_OK status bit has been
> set. The VIRTIO 1.0 specification says, "The device MUST NOT consume
> buffers before DRIVER_OK, and the driver MUST NOT notify the device
> before it sets DRIVER_OK" (3.1 Device Initialization).
>
> http://docs.oasis-open.org/virtio/virtio/v1.0/csprd01/virtio-v1.0-csprd01.html#x1-230001
>
> However, it also says "legacy device implementations often used the
> device before setting the DRIVER_OK bit" (3.1.1 Legacy Interface: Device
> Initialization).
>
> VIRTIO 1.0 can be supported fine by the current librte_vhost API.
> Legacy cannot be supported without API changes - there is no magic way
> to detect when new_device() can be invoked if the driver might require
> some virtqueue processing before the device is fully initialized.
>
> I think this is the main discussion that needs to happen. This patch
> series and the original VIRTIO_NET_F_MQ fix are just workarounds for the
> real problem.
Yes.
>> In this case, the fix I suggested yesterday would work:
>> if ((vhost_features & (1ULL << VIRTIO_NET_F_MQ)) &&
>> !(dev->features & (1ULL << VIRTIO_NET_F_MQ)) {
>> ...
>> }
>>
>> For any backend that does not advertise the feature, no queues will be
>> destroyed.
>
> The feature bit space is shared by all device types. Another device can
> use bit 22 (VIRTIO_NET_F_MQ) for another purpose. This code would
> incorrectly assume it's a net device.
Thanks for pointing this out, I missed that.
> No other device type in VIRTIO 1.0 uses bit 22 yet, but this solution is
> not future-proof. If you decide to use your fix, please include a
> comment in the code so whoever needs to debug this again in the future
> can spot the problem more quickly.
No, I agree this is not future proof. I now think your patch is better.
Thanks for the insights!
Maxime
> Stefan
>