On 11/24/2016 04:24 PM, Kavanagh, Mark B wrote: >> >> On 11/24/2016 12:47 PM, Maxime Coquelin wrote: >>> >>> >>> On 11/24/2016 01:33 PM, Yuanhan Liu wrote: >>>> On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote: >>>>>> On 11/24/2016 06:31 AM, Yuanhan Liu wrote: >>>>>>>> On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote: >>>>>>>>>>>>>> You keep assuming that you have the VM started first and >>>>>>>>>>>>>> figure out things afterwards, but this does not work. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Think about a cluster of machines. You want to start a VM in >>>>>>>>>>>>>> a way that will ensure compatibility with all hosts >>>>>>>>>>>>>> in a cluster. >>>>>>>>>>>> >>>>>>>>>>>> I see. I was more considering about the case when the dst >>>>>>>>>>>> host (including the qemu and dpdk combo) is given, and >>>>>>>>>>>> then determine whether it will be a successfull migration >>>>>>>>>>>> or not. >>>>>>>>>>>> >>>>>>>>>>>> And you are asking that we need to know which host could >>>>>>>>>>>> be a good candidate before starting the migration. In such >>>>>>>>>>>> case, we indeed need some inputs from both the qemu and >>>>>>>>>>>> vhost-user backend. >>>>>>>>>>>> >>>>>>>>>>>> For DPDK, I think it could be simple, just as you said, it >>>>>>>>>>>> could be either a tiny script, or even a macro defined in >>>>>>>>>>>> the source code file (we extend it every time we add a >>>>>>>>>>>> new feature) to let the libvirt to read it. Or something >>>>>>>>>>>> else. >>>>>>>>>> >>>>>>>>>> There's the issue of APIs that tweak features as Maxime >>>>>>>>>> suggested. >>>>>>>> >>>>>>>> Yes, it's a good point. >>>>>>>> >>>>>>>>>> Maybe the only thing to do is to deprecate it, >>>>>>>> >>>>>>>> Looks like so. >>>>>>>> >>>>>>>>>> but I feel some way for application to pass info into >>>>>>>>>> guest might be benefitial. >>>>>>>> >>>>>>>> The two APIs are just for tweaking feature bits DPDK supports >>>>>> before >>>>>>>> any device got connected. It's another way to disable some features >>>>>>>> (the another obvious way is to through QEMU command lines). >>>>>>>> >>>>>>>> IMO, it's bit handy only in a case like: we have bunch of VMs. >>>>>> Instead >>>>>>>> of disabling something though qemu one by one, we could disable it >>>>>>>> once in DPDK. >>>>>>>> >>>>>>>> But I doubt the useful of it. It's only used in DPDK's vhost >>>>>> example >>>>>>>> after all. Nor is it used in vhost pmd, neither is it used in OVS. >>>>>> >>>>>> rte_vhost_feature_disable() is currently used in OVS, >>>>> lib/netdev-dpdk.c >>>> Hmmm. I must have checked very old code ... >>>>>> >>>>>> netdev_dpdk_vhost_class_init(void) >>>>>> { >>>>>> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>>>>> >>>>>> /* This function can be called for different classes. The >>>>>> initialization >>>>>> * needs to be done only once */ >>>>>> if (ovsthread_once_start(&once)) { >>>>>> 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); >>>> I saw the commit introduced such change, but it tells no reason why >>>> it was added. >>> >>> I'm also interested to know the reason. >> >> I can't remember off hand, added Mark K or Michal W who should be able >> to shed some light on it. > > DPDK v16.04 added support for vHost User TSO; as such, by default, TSO is > advertised to guest devices as an available feature during feature > negotiation with QEMU. > However, while the vHost user backend sets up the majority of the mbuf fields > that are required for TSO, there is still a reliance on the associated DPDK > application (i.e. in this case OvS-DPDK) to set the remaining flags and/or > offsets. Since OvS-DPDK doesn't currently provide that functionality, it is > necessary to explicitly disable TSO; otherwise, undefined behaviour will > ensue. Thanks Mark for the clarification.
In this case, maybe we could add a DPDK build option to disable Vhost's TSO support, that would be selected for OVS packages? Does that sound reasonable? Cheers, Maxime >> >>> In any case, I think this is something that can/should be managed by >>> the management tool, which should disable it in cmd parameters. >>> >>> Kevin, do you agree? >> >> I think best to find out the reason first. Because if no reason to >> disable in the code, then no need to debate! >> >>> >>> Cheers, >>> Maxime >