On 11/24/2016 03:34 PM, Loftus, Ciara wrote: >> >> >> On 11/24/2016 03:59 PM, Loftus, Ciara wrote: >>>> >>>> On 11/24/2016 03:20 PM, Ciara Loftus wrote: >>>>> This commit announces support for DPDK 16.11. Compaitibilty with DPDK >>>>> v16.07 is not broken yet thanks to only minor code changes being >> needed >>>>> for the upgrade. >>>>> >>>>> Signed-off-by: Ciara Loftus <[email protected]> >>>>> --- >>>>> .travis/linux-build.sh | 2 +- >>>>> INSTALL.DPDK-ADVANCED.rst | 4 ++-- >>>>> INSTALL.DPDK.rst | 18 +++++++++--------- >>>>> NEWS | 1 + >>>>> lib/netdev-dpdk.c | 3 ++- >>>>> 5 files changed, 15 insertions(+), 13 deletions(-) >>>> <snip /> >>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>>>> index de78ddd..7564ad7 100644 >>>>> --- a/lib/netdev-dpdk.c >>>>> +++ b/lib/netdev-dpdk.c >>>>> @@ -2593,7 +2593,8 @@ netdev_dpdk_vhost_class_init(void) >>>>> rte_vhost_driver_callback_register(&virtio_net_device_ops); >>>>> rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4 >>>>> | 1ULL << VIRTIO_NET_F_HOST_TSO6 >>>>> - | 1ULL << VIRTIO_NET_F_CSUM); >>>>> + | 1ULL << VIRTIO_NET_F_CSUM >>>>> + | 1ULL << VIRTIO_RING_F_INDIRECT_DESC); >>>>> ovs_thread_create("vhost_thread", start_vhost_loop, NULL); >>>>> >>>>> ovsthread_once_done(&once); >>>>> >>>> Any reason you disable indirect descs? >>>> Did you measure performance degradation with it? >>> >>> I measured a slight decrease ~-6% for the use case I tested. >> Ok, could you provide more details about this use-case? >> I'm interested to know which cases (don't) benefit of this feature. > > Single flow 64B packets unidirectional loopback test (dpdk -> vhost, vhost -> > dpdk) with testpmd forwarding between vhost ports in the VM. Mergeable > buffers disabled. > >> >>> Either way, if it is to be enabled it should be enabled in a separate patch. >> Well, I'm not sure. >> I think this is not a good way to disable features, because once >> OVS packaged, we cannot enable it anymore. > > My reasoning not to enable it in this patch (other than the degradation) is > because the purpose of this patch is the DPDK upgrade. We could have a second > patch "enable indirect descriptors for vHost" for example, as suggested by > Ilya previously: > https://mail.openvswitch.org/pipermail/ovs-dev/2016-October/324586.html
+1. I guess it might hold up merging of the upgrade patch and the docs being updated if there's still ongoing debate about whether to enable indirect descs. > >> >> It should be the role of the management tool to disable it by default, >> by configuring the Qemu params properly if needed (indirect_desc=off). >> Doing this, it could still be enabled without rebuilding the package if >> it benefits some use-cases. >> >> I would think this is the same for the other features that are disabled, >> as soon as they don't break functionality. If other features break >> functionality, it should be reported so that it gets fixed. >> >> Moreover, we are looking at cross-version migration for vhost currently, >> and rte_vhost_feature_disable is problematic, because it prevent us to >> export virtio features supported by a given DPDK version reliably at >> build time. > > I see your reasoning too. Would like to hear input from other folks in the > community before I respin. Are there any tests where having indirect descriptors improves performance for OVS...0% pkt loss? larger pkt sizes? If we're pretty sure that the disable virtio feature calls will be deprecated for a nice feature like cross-version migration, it might be better to take the pain of a small perf drop or the user having to modify their config now, rather than at a later stage. We already have a case with mrg buffers where the user can change their config from the default to get better performance. > > Thanks, > Ciara > >> >> Cheers, >> Maxime > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
