diff mbox series

[ovs-dev] netdev-dpdk: vhost: disable unsupported offload features.

Message ID 20200210191850.240247-1-fbl@sysclose.org
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] netdev-dpdk: vhost: disable unsupported offload features. | expand

Commit Message

Flavio Leitner Feb. 10, 2020, 7:18 p.m. UTC
Disable ECN and UFO since this is not supported yet. Also, disable
all other features when userspace_tso is not enabled.

Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/netdev-dpdk.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)


This should go to branch-2.13 if passes the review.

Comments

Ilya Maximets Feb. 12, 2020, 3:26 p.m. UTC | #1
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.

> 
> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  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.

> +        }
> +
> +        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);
>
Flavio Leitner Feb. 12, 2020, 7:31 p.m. UTC | #2
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 <fbl@sysclose.org>
> > ---
> >  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.

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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Flavio Leitner Feb. 12, 2020, 11:44 p.m. UTC | #3
On Wed, Feb 12, 2020 at 04:31:09PM -0300, 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.

BTW, why do you think there is a problem with UDP checksum?
We set VIRTIO_NET_HDR_F_NEEDS_CSUM which should be all it is needed.
Ilya Maximets Feb. 13, 2020, 10:10 a.m. UTC | #4
On 2/13/20 12:44 AM, Flavio Leitner wrote:
> On Wed, Feb 12, 2020 at 04:31:09PM -0300, 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.
> 
> BTW, why do you think there is a problem with UDP checksum?
> We set VIRTIO_NET_HDR_F_NEEDS_CSUM which should be all it is needed.

At least we're sending UDP packets on a wire without
requesting Tx UDP checksum offloading from HW NICs.
Flavio Leitner Feb. 13, 2020, 11:30 a.m. UTC | #5
On Thu, Feb 13, 2020 at 11:10:15AM +0100, Ilya Maximets wrote:
> On 2/13/20 12:44 AM, Flavio Leitner wrote:
> > On Wed, Feb 12, 2020 at 04:31:09PM -0300, 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.
> > 
> > BTW, why do you think there is a problem with UDP checksum?
> > We set VIRTIO_NET_HDR_F_NEEDS_CSUM which should be all it is needed.
> 
> At least we're sending UDP packets on a wire without
> requesting Tx UDP checksum offloading from HW NICs.

Packets from virtual machines relies on DPDK vhost library, and as
far as I can see, it does set PKT_TX_UDP_CKSUM properly.

Now for packets coming from the kernel, then netdev_linux_parse_vnet_hdr()
will set that as well.

Packets from userspace to kernel are properly translated by
netdev_linux_prepend_vnet_hdr().
Ilya Maximets Feb. 13, 2020, 12:42 p.m. UTC | #6
On 2/13/20 12:30 PM, Flavio Leitner wrote:
> On Thu, Feb 13, 2020 at 11:10:15AM +0100, Ilya Maximets wrote:
>> On 2/13/20 12:44 AM, Flavio Leitner wrote:
>>> On Wed, Feb 12, 2020 at 04:31:09PM -0300, 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.
>>>
>>> BTW, why do you think there is a problem with UDP checksum?
>>> We set VIRTIO_NET_HDR_F_NEEDS_CSUM which should be all it is needed.
>>
>> At least we're sending UDP packets on a wire without
>> requesting Tx UDP checksum offloading from HW NICs.
> 
> Packets from virtual machines relies on DPDK vhost library, and as
> far as I can see, it does set PKT_TX_UDP_CKSUM properly.

vhost library will properly set flags, however, OVS doesn't care
about this case, e.g.  we're not enabling DEV_TX_OFFLOAD_UDP_CKSUM
while configuring physical devices.  Even though most of the DPDK
drivers will enable UDP checksum offload support regardless of this
flag, this is still a logical error and we can't rely on this
behavior.

> 
> Now for packets coming from the kernel, then netdev_linux_parse_vnet_hdr()
> will set that as well.
> 
> Packets from userspace to kernel are properly translated by
> netdev_linux_prepend_vnet_hdr().
>
Flavio Leitner Feb. 13, 2020, 1:01 p.m. UTC | #7
On Thu, Feb 13, 2020 at 01:42:32PM +0100, Ilya Maximets wrote:
> On 2/13/20 12:30 PM, Flavio Leitner wrote:
> > On Thu, Feb 13, 2020 at 11:10:15AM +0100, Ilya Maximets wrote:
> >> On 2/13/20 12:44 AM, Flavio Leitner wrote:
> >>> On Wed, Feb 12, 2020 at 04:31:09PM -0300, 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.
> >>>
> >>> BTW, why do you think there is a problem with UDP checksum?
> >>> We set VIRTIO_NET_HDR_F_NEEDS_CSUM which should be all it is needed.
> >>
> >> At least we're sending UDP packets on a wire without
> >> requesting Tx UDP checksum offloading from HW NICs.
> > 
> > Packets from virtual machines relies on DPDK vhost library, and as
> > far as I can see, it does set PKT_TX_UDP_CKSUM properly.
> 
> vhost library will properly set flags, however, OVS doesn't care
> about this case, e.g.  we're not enabling DEV_TX_OFFLOAD_UDP_CKSUM
> while configuring physical devices.  Even though most of the DPDK
> drivers will enable UDP checksum offload support regardless of this
> flag, this is still a logical error and we can't rely on this
> behavior.

Ah, now I see what you're saying.

I think we can support UDP csum, so will post another separate patch
fixing it.

Thanks,
Ilya Maximets Feb. 13, 2020, 3:18 p.m. UTC | #8
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 <fbl@sysclose.org>
>>> ---
>>>  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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Flavio Leitner Feb. 13, 2020, 5:51 p.m. UTC | #9
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 <fbl@sysclose.org>
> >>> ---
> >>>  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,
diff mbox series

Patch

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;
+        }
+
+        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);