[ovs-dev] Vhost-user dequeue zero copy removal

[Kevin Traynor]

On 02/07/2020 12:18, Stokes, Ian wrote:
> 
> 
> On 7/1/2020 3:08 PM, Kevin Traynor wrote:
>> On 01/07/2020 13:46, Ilya Maximets wrote:
>>> On 7/1/20 1:46 PM, Kevin Traynor wrote:
>>>> On 01/07/2020 11:28, Stokes, Ian wrote:
>>>>> Hi All,
>>>>>
>>>>> While completing validation work for DPDK 18.11.9 and 19.11.3 it was
>>>>> found that zero-copy for vhostuserclient devices is no longer possible.
>>>>> Please see commit below from 18.11.9 (note this patch is also in DPDK
>>>>> 19.11.3)
>>>>>
>>>>
>>>> Thanks catching this in your validation, it almost certainly wouldn't
>>>> have been caught otherwise.
>>>
>>> Definitely a good catch.  Thanks.
>>>
>>>>
>>>>>
>>>>> commit 81e025d7ed6a802845909df6fb90505508dc0fbf
>>>>> Author: Xuan Ding <xuan.ding at intel.com>
>>>>> Date:   Wed Apr 29 02:59:46 2020 +0000
>>>>>
>>>>>       vhost: prevent zero-copy with incompatible client mode
>>>>>
>>>>>       [ upstream commit 715070ea10e6da1169deef2a3ea77ae934b4c333 ]
>>>>>
>>>>>       In server mode, virtio-user inits under the assumption that vhost-user
>>>>>       supports a list of features. However, this could be problematic when
>>>>>       in_order feature is negotiated but not supported by vhost-user when
>>>>>       enables dequeue_zero_copy later.
>>>>>
>>>>>       Add handling when vhost-user enables dequeue_zero_copy as client.
>>>
>>> IIUC, this patch basically drops the feature for perfectly fine cases
>>> with VMs.  While it was intended to forbid running zero-copy with virtio-user
>>> it breaks a different usecase blocking the feature entirely.
>>>
>>> Isn't it an API breakage?  IMHO, it should not have been backported in the
>>> first place, since dropping the feature is not what usually expected in
>>> stable releases.  And this must be in release notes anyway.
>>>
>>> I think, the right solution here should be to make a patch to handle specific
>>> virtio-user case and stop blocking valid cases and release new DPDK stable
>>> versions for already released ones.
>>>
>>> If it's too hard to make a patch or no-one wants to work on this, just revert
>>> these changes from stable branches and release a new stable DPDK version
>>> for both 18.11 and 19.11.  But anyway, regression should be addressed in DPDK
>>> before 20.11 or it will block OVS upgrade to that version.
>>>
>>
>> It is not in a released 18.11. It was caught by Ian's team as part of
>> 18.11.9-rc testing.
>>
> 
> OK so it seems like we can use 18.11.9 for the 2.11 and 2.13 branches as 
> it will have this patch reverted.
> 
> Is there an 18.11.9 RC3 planned? We can test it if it's of use.
> 

I've just pushed it to the 18:11 branch, can you test with that?

I wasn't planning on an rc3 at this stage for just this patch, but if
you need a tarball for testing automation or something, let me know and
I can create it.

>>>
>>>>>
>>>>> OVS only supports zero-copy to date as an experimental feature with
>>>>> dpdkvhostuserclient port types.
>>>>>
>>>>> We were aiming to update the validated DPDK versions as follows and
>>>>> recommend them as minimum versions due to the inclusion of CSE fixes.
>>>>>
>>>>> OVS 2.11 -> 18.11.6 -> 18.11.9
>>>>> OVS 2.12 -> 18.11.6 -> 18.11.9
>>>>> OVS 2.13 -> 19.11.0 -> 19.11.3
>>>>> OVS Master -> 19.11.0 -> 19.11.3
>>>>>
>>>>> However recommending these DPDK version will trigger the dpdk zero copy
>>>>> functionality break in OVS.
>>>>>
>>>>
>>>> 18.11.9 is not released yet, so at least for it, I think we could
>>>> replace that patch(es) with a warning.
>>>>
>>>> That is not removing any functionality or causing a regression for users
>>>> of earlier 18.11.x or OVS 2.11/2.12, but it is letting them know there
>>>> may be an issue.
>>>
>>> That might be an option.  But we likely need same change on 19.11 that
>>> will require one more stable release on it.
>>>
>>
>> Sent patches to remove restriction and add warning for 18.11 and 19.11
>> branches here:
>> http://inbox.dpdk.org/stable/20200701135250.20262-1-ktraynor@redhat.com/
>> http://inbox.dpdk.org/stable/20200701135305.20343-1-ktraynor@redhat.com/
>>
>> If there is a better fix in the future, I can apply it in a future release.
>>
>>>>
>>>>          if (vsocket->dequeue_zero_copy) {
>>>>                  if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
>>>> -                       RTE_LOG(ERR, VHOST_CONFIG,
>>>> -                       "error: zero copy is incompatible with vhost
>>>> client mode\n");
>>>> -                       ret = -1;
>>>> -                       goto out_mutex;
>>>> +                       RTE_LOG(WARNING, VHOST_CONFIG,
>>>> +                       "zero copy may be incompatible with vhost client
>>>> mode\n");
>>>>                  }
>>>>
>>>>> What are peoples thoughts here on how to proceed?
>>>>> Are people aware if the feature is used in deployments to date? If not,
>>>>> as it's experimental is it something that should be removed?
>>>
>>> I don't think that we should remove the feature from OVS since it, IIUC,
>>> could be enabled back in DPDK.
> 
> Agreed with above, if it will be fixed in DPDK for 18.11 and a future 
> 19.11 then we can use them.
> 
> In this case I will propose that we should move to recommend 19.11.2 
> instead of 19.11.3 at the moment for OVS 2.13 and OVS master. 19.11.2 
> has the vhost CVE security fixes and has been validated on our side 
> already. Once 19.11.4 is released we could then move to that if the zero 
> copy patch was resolved?
> 

Sounds good to me. Luca has accepted the patch I sent yesterday for
19.11 branch, so it is due to be in 19.11.4.


> Would this be acceptable? Or are there any critical patches in 19.11.3 
> that we would be missing?
> 

There are no CVE fixes added in 19.11.3 at least

> BR
> Ian
> 
>