From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH v2] virtio: enable indirect descriptors feature Date: Mon, 5 Sep 2016 16:24:13 +0200 Message-ID: <1dfca8cf-e5ef-1040-8a40-d617ed03e5c0@redhat.com> References: <516F65D3-4706-4CC5-916B-6ECD29CBE177@cisco.com> <20160905022028.GF30752@yliu-dev.sh.intel.com> <6EFF45F1-172E-4470-B4D7-AED90474DE50@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Cc: "dev@dpdk.org" To: "Pierre Pfister (ppfister)" , Yuanhan Liu Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id F135E5681 for ; Mon, 5 Sep 2016 16:24:17 +0200 (CEST) In-Reply-To: <6EFF45F1-172E-4470-B4D7-AED90474DE50@cisco.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" Thanks Pierre for sending the fix. Minor comments below: 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. > You should add the below line: Fixes: 4a92b671 ("virtio: clarify feature bit handling") 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. Other than that, you can add: Reviewed-by: Maxime Coquelin > Signed-off-by: Pierre Pfister > --- > drivers/net/virtio/virtio_ethdev.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h > index 2ecec6e..31c91a5 100644 > --- a/drivers/net/virtio/virtio_ethdev.h > +++ b/drivers/net/virtio/virtio_ethdev.h > @@ -63,6 +63,7 @@ > 1u << VIRTIO_NET_F_CTRL_RX | \ > 1u << VIRTIO_NET_F_CTRL_VLAN | \ > 1u << VIRTIO_NET_F_MRG_RXBUF | \ > + 1u << VIRTIO_RING_F_INDIRECT_DESC | \ > 1ULL << VIRTIO_F_VERSION_1) > > /* > -- > 2.7.4 (Apple Git-66) > One off-topic question, do you measure some improvement in perfs using the feature? If yes, could you describe the use-case, and the figures? I ask because I have implemented TX indirect descriptor in vhost lib (see http://dpdk.org/dev/patchwork/patch/14797/), and failed to see some use-case benefiting of it. Regards, Maxime