On Thu, Feb 13, 2020 at 04:18:35PM +0100, Ilya Maximets wrote:
> 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;
I will respin the patch as part of series fixing csum as well.
Thanks,
--
fbl
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
