[ovs-dev] [PATCH v4 3/3] netdev-dpdk: Add TCP Segmentation Offload support

Flavio Leitner fbl at sysclose.org
Fri Jan 17 19:58:57 UTC 2020


On Fri, Jan 17, 2020 at 06:58:56PM +0100, Ilya Maximets wrote:
> On 16.01.2020 18:00, Flavio Leitner wrote:
> > Abbreviated as TSO, TCP Segmentation Offload is a feature which enables
> > the network stack to delegate the TCP segmentation to the NIC reducing
> > the per packet CPU overhead.
> > 
> > A guest using vhostuser interface with TSO enabled can send TCP packets
> > much bigger than the MTU, which saves CPU cycles normally used to break
> > the packets down to MTU size and to calculate checksums.
> > 
> > It also saves CPU cycles used to parse multiple packets/headers during
> > the packet processing inside virtual switch.
> > 
> > If the destination of the packet is another guest in the same host, then
> > the same big packet can be sent through a vhostuser interface skipping
> > the segmentation completely. However, if the destination is not local,
> > the NIC hardware is instructed to do the TCP segmentation and checksum
> > calculation.
> > 
> > It is recommended to check if NIC hardware supports TSO before enabling
> > the feature, which is off by default. For additional information please
> > check the tso.rst document.
> > 
> > Signed-off-by: Flavio Leitner <fbl at sysclose.org>
> 
> 
> I still didn't check computation of offsets and device configuration.
> Few comments inline.
> 
> In general, I'd like if Ben will take a look to this patch, especially
> at extensive netdev-linux code changes.
> 
> Also, suggesting to rename the patch to:
> 'userspace: Add TCP Segmentation Offload support.'

Will do a V5 with all the minor fixes listed below.

The ones that you suggested to do in another patch I also agree
and left as is.

[...]
> > @@ -1454,9 +1595,15 @@ netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED,
> >                    struct dp_packet_batch *batch,
> >                    bool concurrent_txq OVS_UNUSED)
> >  {
> > +    bool tso = userspace_tso_enabled();
> > +    int mtu = ETH_PAYLOAD_MAX;
> >      int error = 0;
> >      int sock = 0;
> >  
> > +    if (tso) {
> > +        netdev_linux_get_mtu__(netdev_linux_cast(netdev_), &mtu);
> 
> netdev_linux_get_mtu__() could fail.  This needs to be handled.

If that fails, the mtu is not changed, so we use the ETH_PAYLOAD_MAX
as a fall back.

[...]
> > @@ -6234,3 +6393,136 @@ af_packet_sock(void)
> >  
> >      return sock;
> >  }
> > +
> > +static int
> > +netdev_linux_parse_l2(struct dp_packet *b, uint16_t *l4proto)
> 
> I'm not generally a fan of parsing packets here on receive especially
> because we will parse it with miniflow_extract() later, but I'm not
> sure how to avoid that.

Maybe miniflow_extract() could use hints provided by this parser.

[...] 
> > diff --git a/lib/userspace-tso.c b/lib/userspace-tso.c
> > new file mode 100644
> > index 000000000..f843c2a76
> > --- /dev/null
> > +++ b/lib/userspace-tso.c
> > @@ -0,0 +1,48 @@
> > +/*
> > + * Copyright (c) 2020 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "smap.h"
> > +#include "ovs-thread.h"
> > +#include "openvswitch/vlog.h"
> > +#include "dpdk.h"
> > +#include "userspace-tso.h"
> > +#include "vswitch-idl.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(userspace_tso);
> > +
> > +static bool userspace_tso = false;
> > +
> > +void
> > +userspace_tso_init(const struct smap *ovs_other_config)
> > +{
> > +    if (smap_get_bool(ovs_other_config, "userspace-tso-enable", false)) {
> > +        static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> > +
> > +        if (ovsthread_once_start(&once)) {
> > +            VLOG_INFO("Userspace TCP Segmentation Offloading support enabled");
> > +            userspace_tso = true;
> 
> Since dp_packet functions has no implementation if OVS built without
> DPDK support, I think, we need to restrict enabling the functionality.

I will add the restriction back.

> #ifdef DPDK_NETDEV
>             VLOG_INFO("Userspace TCP Segmentation Offloading support enabled");
>             userspace_tso = true;
> #else
>             VLOG_WARN("Userspace TCP Segmentation Offloading can not be enabled"
>                       "since OVS built without DPDK support.");
> #endif
> 

Thanks,
-- 
fbl


More information about the dev mailing list