Re: [dpdk-dev] dpdk/vpp and cross-version migration for vhost

["Kavanagh, Mark B" <mark.b.kavanagh@intel.com>]

>
>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.

>
>> 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