On 2/12/20 8:31 PM, Flavio Leitner wrote:
> On Wed, Feb 12, 2020 at 04:26:46PM +0100, Ilya Maximets wrote:
>> On 2/10/20 8:18 PM, Flavio Leitner wrote:
>>> Disable ECN and UFO since this is not supported yet. Also, disable
>>> all other features when userspace_tso is not enabled.
>>
>> Disabling UFO saves us from receiving of a big unfragmented
>> UDP packets, but UDP packets will still have bad checksums
>> and will be dropped by the kernel on receive side.
>> So, this is only part of a fix.
>
> Yes, this patch is not intended to fix the UDP checksum problem.
> Unless you see a way to do that with changing the flags below.
>
>>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
>>> Signed-off-by: Flavio Leitner <[email protected]>
>>> ---
>>> lib/netdev-dpdk.c | 25 ++++++++++++++++---------
>>> 1 file changed, 16 insertions(+), 9 deletions(-)
>>>
>>>
>>> This should go to branch-2.13 if passes the review.
>>>
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 6187129c0..b3983b312 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -5186,6 +5186,7 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
>>> *netdev)
>>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>> int err;
>>> uint64_t vhost_flags = 0;
>>> + uint64_t vhost_unsup_flags;
>>> bool zc_enabled;
>>>
>>> ovs_mutex_lock(&dev->mutex);
>>> @@ -5252,16 +5253,22 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
>>> *netdev)
>>> netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO;
>>> netdev->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM;
>>> netdev->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
>>> + vhost_unsup_flags = 1ULL << VIRTIO_NET_F_HOST_ECN
>>> + | 1ULL << VIRTIO_NET_F_HOST_UFO;
>>> } else {
>>> - err = rte_vhost_driver_disable_features(dev->vhost_id,
>>> - 1ULL << VIRTIO_NET_F_HOST_TSO4
>>> - | 1ULL << VIRTIO_NET_F_HOST_TSO6
>>> - | 1ULL << VIRTIO_NET_F_CSUM);
>>> - if (err) {
>>> - VLOG_ERR("rte_vhost_driver_disable_features failed for "
>>> - "vhost user client port: %s\n", dev->up.name);
>>> - goto unlock;
>>> - }
>>> + vhost_unsup_flags = 1ULL << VIRTIO_NET_F_CSUM
>>> + | 1ULL << VIRTIO_NET_F_HOST_UFO
>>> + | 1ULL << VIRTIO_NET_F_HOST_TSO4
>>> + | 1ULL << VIRTIO_NET_F_HOST_TSO6
>>> + | 1ULL << VIRTIO_NET_F_HOST_ECN;
>>
>> All above features depends on VIRTIO_NET_F_CSUM, so, I think, we only
>> need to disable this one.
>
> You right. I added the others flags anyways to make it explicit. I thought
> either a comment or code would be needed. Since I see people grep'ing
> for the flags, there you go.
I think, it's better to add a comment like this:
/* This disables checksum offloading and all the features
* that depends on it (TSO, UFO, ECN) according to virtio
* specification. */
vhost_unsup_flags = 1ULL << VIRTIO_NET_F_CSUM;
>
> fbl
>
>>
>>> + }
>>> +
>>> + err = rte_vhost_driver_disable_features(dev->vhost_id,
>>> + vhost_unsup_flags);
>>> + if (err) {
>>> + VLOG_ERR("rte_vhost_driver_disable_features failed for "
>>> + "vhost user client port: %s\n", dev->up.name);
>>> + goto unlock;
>>> }
>>>
>>> err = rte_vhost_driver_start(dev->vhost_id);
>>>
>>
>> _______________________________________________
>> 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
