From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH v2] virtio: enable indirect descriptors feature Date: Mon, 5 Sep 2016 14:08:24 -0700 Message-ID: <20160905140824.1f0c80a2@xeon-e3> References: <516F65D3-4706-4CC5-916B-6ECD29CBE177@cisco.com> <20160905022028.GF30752@yliu-dev.sh.intel.com> <6EFF45F1-172E-4470-B4D7-AED90474DE50@cisco.com> <1dfca8cf-e5ef-1040-8a40-d617ed03e5c0@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: "Pierre Pfister (ppfister)" , Yuanhan Liu , "dev@dpdk.org" To: Maxime Coquelin Return-path: Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com [209.85.220.42]) by dpdk.org (Postfix) with ESMTP id 909654A63 for ; Mon, 5 Sep 2016 23:08:14 +0200 (CEST) Received: by mail-pa0-f42.google.com with SMTP id id6so5819651pad.3 for ; Mon, 05 Sep 2016 14:08:14 -0700 (PDT) In-Reply-To: <1dfca8cf-e5ef-1040-8a40-d617ed03e5c0@redhat.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, 5 Sep 2016 16:24:13 +0200 Maxime Coquelin wrote: > Thanks Pierre for sending the fix. >=20 > Minor comments below: >=20 > On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote: > > Indirect descriptors support was disabled by commit 4a92b67151be11, > > presumably by accident as it was correctly supported. > > > > This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to > > the supported features bit mask, hence enabling the use of > > indirect descriptors when the feature is negociated with the > > device. > > =20 >=20 > You should add the below line: > Fixes: 4a92b671 ("virtio: clarify feature bit handling") >=20 > Also, maybe we should consider add stable@dpdk.org in cc:, > because the regression was introduced before v16.07 final tag. > But the problem is that all the final validation has been done > without this feature enabled, and it impact quite a few lines of > code in Virtio PMD. >=20 > Other than that, you can add: > Reviewed-by: Maxime Coquelin The patch is correct, but it doesn't fix a regression. The original virtio DPDK did not support INDIRECT descriptors at all. The original code in virtio_negotiate features was the inverse of what it i= s now. Read carefully, the values in mask were the bits that were rejected during guest negotiation at the time. static void virtio_negotiate_features(struct virtio_hw *hw) { - uint32_t host_features, mask; - - /* checksum offload not implemented */ - mask =3D VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM; - - /* TSO and LRO are only available when their corresponding - * checksum offload feature is also negotiated. - */ - mask |=3D VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_= HOST_ECN; - mask |=3D VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_= F_GUEST_ECN; - mask |=3D VTNET_LRO_FEATURES; - - /* not negotiating INDIRECT descriptor table support */ - mask |=3D VIRTIO_RING_F_INDIRECT_DESC; + uint32_t host_features; =20 /* Prepare guest_features: feature that driver wants to support */ - hw->guest_features =3D VTNET_FEATURES & ~mask; + hw->guest_features =3D VIRTIO_PMD_GUEST_FEATURES; PMD_INIT_LOG(DEBUG, "guest_features before negotiate =3D %x", hw->guest_features); =20 Therefore INDIRECT descriptors were always disabled! Don't blame any commi= t. Use of indirect descriptors by DPDK did not happen until a later change. commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd Author: Stephen Hemminger Date: Fri Mar 4 10:19:19 2016 -0800 virtio: use indirect ring elements =20 The virtio ring in QEMU/KVM is usually limited to 256 entries and the normal way that virtio driver was queuing mbufs required nsegs + 1 ring elements. By using the indirect ring element feature if available, each packet will take only one ring slot even for multi-segment packets. =20 Signed-off-by: Stephen Hemminger Acked-by: Yuanhan Liu Acked-by: Huawei Xie