diff mbox series

[ovs-dev,v12,3/3] userspace: Fix packet leak and style issues.

Message ID 20240117090725.2333-1-dexia.li@jaguarmicro.com
State Changes Requested
Headers show
Series [ovs-dev,v12,1/3] userspace: Support vxlan and geneve tso. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success

Commit Message

Dexia Li Jan. 17, 2024, 9:07 a.m. UTC 
For userspace vxlan and geneve tunnel tso, this commit
fix one packet leak and modify style issues.

Signed-off-by: Dexia Li <dexia.li@jaguarmicro.com>
---
 lib/dp-packet.c         |  6 +++---
 lib/dp-packet.h         | 46 +++++++++++++++++++++--------------------
 lib/netdev-dpdk.c       |  5 ++---
 lib/netdev-native-tnl.c | 26 +++++++++++------------
 lib/netdev.c            | 28 +++++++++++++++++++------
 5 files changed, 64 insertions(+), 47 deletions(-)

Comments

Ilya Maximets Jan. 17, 2024, 3:37 p.m. UTC | #1 
On 1/17/24 10:07, Dexia Li wrote:
> For userspace vxlan and geneve tunnel tso, this commit
> fix one packet leak and modify style issues.
> 
> Signed-off-by: Dexia Li <dexia.li@jaguarmicro.com>
> ---
>  lib/dp-packet.c         |  6 +++---
>  lib/dp-packet.h         | 46 +++++++++++++++++++++--------------------
>  lib/netdev-dpdk.c       |  5 ++---
>  lib/netdev-native-tnl.c | 26 +++++++++++------------
>  lib/netdev.c            | 28 +++++++++++++++++++------
>  5 files changed, 64 insertions(+), 47 deletions(-)

Hi, Dexia.  Thanks for the update!

Changes look good, except for a few comments below.

Mike, AFAIU, it's already very late for Dexia today, but we need to address
some more issues today before branching for 3.3.  Could you make the changes
proposed below/test and post v13 with them?  Thanks in advance.

(This patch may also be squashed into the first one)

> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index cb20608d7..e7738c37a 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -548,7 +548,7 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct dp_packet *b2,
>  
>  void
>  dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
> -                                uint64_t flags)
> +                                    uint64_t flags)
>  {
>      if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
>          if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
> @@ -558,7 +558,7 @@ dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
>          }
>      }
>  
> -    if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
> +    if (!dp_packet_hwol_is_outer_udp_cksum(p)) {
>          return;
>      }
>  
> @@ -596,7 +596,7 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
>          return;
>      }
>  
> -    if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
> +    if (dp_packet_l4_checksum_good(p) && !tnl_inner) {
>          dp_packet_hwol_reset_tx_l4_csum(p);
>          return;
>      }
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index cf341f09c..c9ec47c31 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -92,16 +92,16 @@ enum dp_packet_offload_mask {
>      /* Offload packet is tunnel VXLAN. */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
>                  RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
> -    /* Offload tunnel packet, out is ipv4 */
> +    /* Offload tunnel packet, out is IPV4 */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
>                  RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
> -    /* Offload TUNNEL out ipv4 checksum */
> +    /* Offload tunnel out IPV4 checksum */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IP_CKSUM,
>                  RTE_MBUF_F_TX_OUTER_IP_CKSUM, 0x10000),
> -    /* Offload TUNNEL out udp checksum */
> +    /* Offload tunnel out UDP checksum */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_UDP_CKSUM,
>                  RTE_MBUF_F_TX_OUTER_UDP_CKSUM, 0x20000),
> -    /* Offload tunnel packet, out is ipv6 */
> +    /* Offload tunnel packet, out is IPV6 */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV6,
>                  RTE_MBUF_F_TX_OUTER_IPV6, 0x40000),
>  
> @@ -164,9 +164,9 @@ struct dp_packet {
>                                      * or UINT16_MAX. */
>      uint16_t l4_ofs;               /* Transport-level header offset,
>                                        or UINT16_MAX. */
> -    uint16_t inner_l3_ofs;         /* inner Network-level header offset,
> +    uint16_t inner_l3_ofs;         /* Inner Network-level header offset,
>                                      * or UINT16_MAX. */
> -    uint16_t inner_l4_ofs;         /* inner Transport-level header offset,
> +    uint16_t inner_l4_ofs;         /* Inner Transport-level header offset,
>                                        or UINT16_MAX. */
>      uint32_t cutlen;               /* length in bytes to cut from the end. */
>      ovs_be32 packet_type;          /* Packet type as defined in OpenFlow */
> @@ -279,8 +279,8 @@ bool dp_packet_compare_offsets(struct dp_packet *good,
>                                 struct dp_packet *test,
>                                 struct ds *err_str);
>  void dp_packet_ol_send_prepare(struct dp_packet *, uint64_t);
> -void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
> -                                         uint64_t flags);
> +void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *,
> +                                         uint64_t);
>  
>  
>  
> @@ -640,19 +640,19 @@ dp_packet_flow_mark_ptr(const struct dp_packet *b)
>  static inline void
>  dp_packet_set_l2_len(struct dp_packet *b OVS_UNUSED, size_t l2_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline void
>  dp_packet_set_l3_len(struct dp_packet *b OVS_UNUSED, size_t l3_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline void
>  dp_packet_set_l4_len(struct dp_packet *b OVS_UNUSED, size_t l4_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline uint32_t *
> @@ -1162,23 +1162,26 @@ dp_packet_hwol_is_tunnel_vxlan(struct dp_packet *b)
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_TUNNEL_VXLAN);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out ipv4. */
> +/* Returns 'true' if packet 'b' is marked as a tunnel
> + * packet with outer header being IPv4. */
>  static inline bool
>  dp_packet_hwol_is_outer_ipv4(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IPV4);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out ipv4 csum offload. */
> +/* Returns 'true' if a tunnel packet 'b' is marked for IPv4 checksum offload
> + * for the outer header. */
>  static inline bool
>  dp_packet_hwol_is_outer_ipv4_cksum(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IP_CKSUM);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out udp csum offload. */
> +/* Returns 'true' if a tunnel packet 'b' is marked for UDP checksum  offload
> + * for the outer header. */
>  static inline bool
> -dp_packet_hwol_is_outer_UDP_cksum(struct dp_packet *b)
> +dp_packet_hwol_is_outer_udp_cksum(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_UDP_CKSUM);
>  }
> @@ -1205,7 +1208,8 @@ dp_packet_hwol_set_tx_ipv6(struct dp_packet *a)
>      *dp_packet_ol_flags_ptr(a) |= DP_PACKET_OL_TX_IPV6;
>  }
>  
> -/* Mark packet 'a' as IPv6. */
> +/* Returns 'true' if packet 'b' is marked as a tunnel
> + * packet with outer header being IPv6. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv6(struct dp_packet *a)
>  {
> @@ -1266,30 +1270,28 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
>  }
>  
> -/* Mark packet 'b' for tunnel geneve offloading.  It implies that
> - * the packet 'b' is marked for tunnel geneve offloading. */
> +/* Mark packet 'b' for tunnel geneve offloading. */
>  static inline void
>  dp_packet_hwol_set_tunnel_geneve(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_GENEVE;
>  }
>  
> -/* Mark packet 'b' for tunnel vxlan offloading.  It implies that
> - * the packet 'b' is marked for tunnel vxlan offloading. */
> +/* Mark packet 'b' for tunnel vxlan offloading. */
>  static inline void
>  dp_packet_hwol_set_tunnel_vxlan(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_VXLAN;
>  }
>  
> -/* Mark packet 'b' for out ipv4 packet. */
> +/* Mark packet 'b' as a tunnel packet with IPv4 outer header. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv4(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_OUTER_IPV4;
>  }
>  
> -/* Mark packet 'b' for out ipv4 csum offloading. */
> +/* Mark packet 'b' for IPv4 checksum offload for the outer header. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv4_csum(struct dp_packet *b)
>  {
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e4d95d0a3..fb26825ff 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1157,7 +1157,6 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>          conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
>      }
>  
> -
>      /* Limit configured rss hash functions to only those supported
>       * by the eth device. */
>      conf.rx_adv_conf.rss_conf.rss_hf &= info.flow_type_rss_offloads;
> @@ -2570,8 +2569,8 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf)
>  
>          if (mbuf->ol_flags & (RTE_MBUF_F_TX_TUNNEL_GENEVE |
>              RTE_MBUF_F_TX_TUNNEL_VXLAN)) {
> -            mbuf->tso_segsz  = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> -            mbuf->l4_len - mbuf->outer_l3_len;
> +            mbuf->tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> +                              mbuf->l4_len - mbuf->outer_l3_len;
>          } else {
>              mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
>              mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 35767800c..d513194dc 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -245,8 +245,8 @@ udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>   * encapsulated now. */
>  static void
>  dp_packet_tnl_ol_process(const struct netdev *netdev,

We need to bring back the OVS_UNUSED here, since it's not longer
used within the function.

> -                             struct dp_packet *packet,
> -                             const struct ovs_action_push_tnl *data)
> +                         struct dp_packet *packet,
> +                         const struct ovs_action_push_tnl *data)
>  {
>      struct udp_header *udp = NULL;
>      uint8_t opt_len = 0;
> @@ -256,8 +256,8 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>  
>      /* l2 l3 l4 len refer to inner len, tunnel outer
>       * header is not encapsulated here. */
> -   if (dp_packet_hwol_l4_mask(packet)) {
> -       ip = dp_packet_l3(packet);
> +    if (dp_packet_hwol_l4_mask(packet)) {
> +        ip = dp_packet_l3(packet);
>  
>          if (ip->ip_proto == IPPROTO_TCP) {
>              struct tcp_header *th = dp_packet_l4(packet);
> @@ -269,10 +269,10 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>          }
>  
>          dp_packet_set_l3_len(packet, (char *) dp_packet_l4(packet) -
> -                              (char *) dp_packet_l3(packet));
> +                                     (char *) dp_packet_l3(packet));
>  
> -        if (!strcmp(netdev_get_type(netdev), "geneve") ||
> -            !strcmp(netdev_get_type(netdev), "vxlan")) {
> +        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE ||
> +            data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
>  
>              if (IP_VER(ip->ip_ihl_ver) == 4) {
>                  dp_packet_hwol_set_tx_ipv4(packet);
> @@ -284,7 +284,7 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>  
>          /* Attention please, tunnel inner l2 len is consist of udp header
>           * len and tunnel header len and inner l2 len. */
> -        if (!strcmp(netdev_get_type(netdev), "geneve")) {
> +        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE) {
>              eth = (struct eth_header *)(data->header);
>              ip = (struct ip_header *)(eth + 1);
>              udp = (struct udp_header *)(ip + 1);
> @@ -292,13 +292,13 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>              opt_len = gnh->opt_len * 4;
>              dp_packet_hwol_set_tunnel_geneve(packet);
>              dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
> -                              (char *) dp_packet_eth(packet) +
> -                              GENEVE_BASE_HLEN + opt_len);
> -        } else if (!strcmp(netdev_get_type(netdev), "vxlan")) {
> +                                         (char *) dp_packet_eth(packet) +
> +                                         GENEVE_BASE_HLEN + opt_len);
> +        } else if (data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
>              dp_packet_hwol_set_tunnel_vxlan(packet);
>              dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
> -                              (char *) dp_packet_eth(packet) +
> -                              VXLAN_HLEN);
> +                                         (char *) dp_packet_eth(packet) +
> +                                         VXLAN_HLEN);
>          }
>      }
>  }
> diff --git a/lib/netdev.c b/lib/netdev.c
> index db0610304..dc3089396 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -70,6 +70,8 @@ COVERAGE_DEFINE(netdev_sent);
>  COVERAGE_DEFINE(netdev_add_router);
>  COVERAGE_DEFINE(netdev_get_stats);
>  COVERAGE_DEFINE(netdev_push_header_drops);
> +COVERAGE_DEFINE(netdev_vxlan_tso_not_support);
> +COVERAGE_DEFINE(netdev_geneve_tso_not_support);

Nit: I'd rename these to netdev_vxlan/geneve_tso_drops, just to be closer
to other drop counter names.

>  COVERAGE_DEFINE(netdev_soft_seg_good);
>  COVERAGE_DEFINE(netdev_soft_seg_drops);
>  
> @@ -915,12 +917,16 @@ netdev_send(struct netdev *netdev, int qid, struct dp_packet_batch *batch,
>                  if (dp_packet_hwol_is_tunnel_vxlan(packet)
>                      && !(netdev_flags & NETDEV_TX_VXLAN_TNL_TSO)) {
>                          VLOG_ERR("No VXLAN TSO support");

We still need to rate-limit these.  And maybe downgrade them to WARN,
since we have a similar warning in the tunnel_push function.  E.g.:

VLOG_WARN_RL(&rl, "port '%s' doesn't support VXLAN TSO, packet dropped",
             netdev_get_name(netdev));

> +                        COVERAGE_INC(netdev_vxlan_tso_not_support);
> +                        dp_packet_delete(packet);

We need to free the whole batch here, not only one packet.

>                          return false;
>                  }
>  
>                  if (dp_packet_hwol_is_tunnel_geneve(packet)
>                      && !(netdev_flags & NETDEV_TX_GENEVE_TNL_TSO)) {
>                          VLOG_ERR("No GENEVE TSO support");

Ditto.

> +                        COVERAGE_INC(netdev_geneve_tso_not_support);
> +                        dp_packet_delete(packet);

Ditto.

>                          return false;
>                  }
>                  return netdev_send_tso(netdev, qid, batch, concurrent_txq);
> @@ -1001,19 +1007,29 @@ netdev_push_header(const struct netdev *netdev,
>      size_t i, size = dp_packet_batch_size(batch);
>  
>      DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> -        if (OVS_UNLIKELY(strcmp(netdev_get_type(netdev), "vxlan") &&
> -            strcmp(netdev_get_type(netdev), "geneve") &&
> +        if (OVS_UNLIKELY(data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
> +            data->tnl_type != OVS_VPORT_TYPE_VXLAN &&
>              dp_packet_hwol_is_tso(packet))) {
>              COVERAGE_INC(netdev_push_header_drops);
>              dp_packet_delete(packet);
> -            VLOG_WARN_RL(&rl, "%s: Tunneling packets with tso HW offload"
> -                     "flags is not supported: packet dropped",
> +            VLOG_WARN_RL(&rl, "Tunneling packets with TSO is not supported"
> +                     "for %s tunnels, packet dropped",
>                       netdev_get_name(netdev));
>          } else {
> -            if (strcmp(netdev_get_type(netdev), "vxlan") &&
> -                strcmp(netdev_get_type(netdev), "geneve")) {
> +            if (OVS_UNLIKELY(dp_packet_hwol_is_tunnel_geneve(packet) ||
> +                dp_packet_hwol_is_tunnel_vxlan(packet))) {

I'd shift this 13 spaces to the right.

> +                COVERAGE_INC(netdev_push_header_drops);
> +                dp_packet_delete(packet);
> +                VLOG_WARN_RL(&rl, "%s tunnel tso packet before encap"
> +                         "not support, packet dropped",
> +                         netdev_get_name(netdev));

Maybe just "Double tunnel encapsulation with TSO is not supported." ?

Also, we dropped the packet, so we need to 'continue;' here, so we will
not use it after free.

> +            }
> +
> +            if (data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
> +                data->tnl_type != OVS_VPORT_TYPE_VXLAN) {
>                  dp_packet_ol_send_prepare(packet, 0);
>              }
> +
>              netdev->netdev_class->push_header(netdev, packet, data);
>  
>              pkt_metadata_init(&packet->md, data->out_port);
Simon Horman Jan. 17, 2024, 5:31 p.m. UTC | #2 
On Wed, Jan 17, 2024 at 05:07:25PM +0800, Dexia Li via dev wrote:
> For userspace vxlan and geneve tunnel tso, this commit
> fix one packet leak and modify style issues.
> 
> Signed-off-by: Dexia Li <dexia.li@jaguarmicro.com>
> ---
>  lib/dp-packet.c         |  6 +++---
>  lib/dp-packet.h         | 46 +++++++++++++++++++++--------------------
>  lib/netdev-dpdk.c       |  5 ++---
>  lib/netdev-native-tnl.c | 26 +++++++++++------------
>  lib/netdev.c            | 28 +++++++++++++++++++------
>  5 files changed, 64 insertions(+), 47 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index cb20608d7..e7738c37a 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -548,7 +548,7 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct dp_packet *b2,
>  
>  void
>  dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
> -                                uint64_t flags)
> +                                    uint64_t flags)
>  {
>      if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
>          if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
> @@ -558,7 +558,7 @@ dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
>          }
>      }
>  
> -    if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
> +    if (!dp_packet_hwol_is_outer_udp_cksum(p)) {
>          return;
>      }
>  
> @@ -596,7 +596,7 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
>          return;
>      }
>  
> -    if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
> +    if (dp_packet_l4_checksum_good(p) && !tnl_inner) {
>          dp_packet_hwol_reset_tx_l4_csum(p);
>          return;
>      }
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index cf341f09c..c9ec47c31 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -92,16 +92,16 @@ enum dp_packet_offload_mask {
>      /* Offload packet is tunnel VXLAN. */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
>                  RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
> -    /* Offload tunnel packet, out is ipv4 */
> +    /* Offload tunnel packet, out is IPV4 */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
>                  RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
> -    /* Offload TUNNEL out ipv4 checksum */
> +    /* Offload tunnel out IPV4 checksum */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IP_CKSUM,
>                  RTE_MBUF_F_TX_OUTER_IP_CKSUM, 0x10000),
> -    /* Offload TUNNEL out udp checksum */
> +    /* Offload tunnel out UDP checksum */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_UDP_CKSUM,
>                  RTE_MBUF_F_TX_OUTER_UDP_CKSUM, 0x20000),
> -    /* Offload tunnel packet, out is ipv6 */
> +    /* Offload tunnel packet, out is IPV6 */
>      DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV6,
>                  RTE_MBUF_F_TX_OUTER_IPV6, 0x40000),
>  
> @@ -164,9 +164,9 @@ struct dp_packet {
>                                      * or UINT16_MAX. */
>      uint16_t l4_ofs;               /* Transport-level header offset,
>                                        or UINT16_MAX. */
> -    uint16_t inner_l3_ofs;         /* inner Network-level header offset,
> +    uint16_t inner_l3_ofs;         /* Inner Network-level header offset,
>                                      * or UINT16_MAX. */
> -    uint16_t inner_l4_ofs;         /* inner Transport-level header offset,
> +    uint16_t inner_l4_ofs;         /* Inner Transport-level header offset,
>                                        or UINT16_MAX. */
>      uint32_t cutlen;               /* length in bytes to cut from the end. */
>      ovs_be32 packet_type;          /* Packet type as defined in OpenFlow */
> @@ -279,8 +279,8 @@ bool dp_packet_compare_offsets(struct dp_packet *good,
>                                 struct dp_packet *test,
>                                 struct ds *err_str);
>  void dp_packet_ol_send_prepare(struct dp_packet *, uint64_t);
> -void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
> -                                         uint64_t flags);
> +void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *,
> +                                         uint64_t);
>  
>  
>  
> @@ -640,19 +640,19 @@ dp_packet_flow_mark_ptr(const struct dp_packet *b)
>  static inline void
>  dp_packet_set_l2_len(struct dp_packet *b OVS_UNUSED, size_t l2_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline void
>  dp_packet_set_l3_len(struct dp_packet *b OVS_UNUSED, size_t l3_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline void
>  dp_packet_set_l4_len(struct dp_packet *b OVS_UNUSED, size_t l4_len OVS_UNUSED)
>  {
> -    /* There are no implementation */
> +    /* There is no implementation. */
>  }
>  
>  static inline uint32_t *
> @@ -1162,23 +1162,26 @@ dp_packet_hwol_is_tunnel_vxlan(struct dp_packet *b)
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_TUNNEL_VXLAN);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out ipv4. */
> +/* Returns 'true' if packet 'b' is marked as a tunnel
> + * packet with outer header being IPv4. */
>  static inline bool
>  dp_packet_hwol_is_outer_ipv4(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IPV4);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out ipv4 csum offload. */
> +/* Returns 'true' if a tunnel packet 'b' is marked for IPv4 checksum offload
> + * for the outer header. */
>  static inline bool
>  dp_packet_hwol_is_outer_ipv4_cksum(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IP_CKSUM);
>  }
>  
> -/* Returns 'true' if packet 'b' is marked for out udp csum offload. */
> +/* Returns 'true' if a tunnel packet 'b' is marked for UDP checksum  offload
> + * for the outer header. */
>  static inline bool
> -dp_packet_hwol_is_outer_UDP_cksum(struct dp_packet *b)
> +dp_packet_hwol_is_outer_udp_cksum(struct dp_packet *b)
>  {
>      return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_UDP_CKSUM);
>  }
> @@ -1205,7 +1208,8 @@ dp_packet_hwol_set_tx_ipv6(struct dp_packet *a)
>      *dp_packet_ol_flags_ptr(a) |= DP_PACKET_OL_TX_IPV6;
>  }
>  
> -/* Mark packet 'a' as IPv6. */
> +/* Returns 'true' if packet 'b' is marked as a tunnel
> + * packet with outer header being IPv6. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv6(struct dp_packet *a)
>  {
> @@ -1266,30 +1270,28 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
>  }
>  
> -/* Mark packet 'b' for tunnel geneve offloading.  It implies that
> - * the packet 'b' is marked for tunnel geneve offloading. */
> +/* Mark packet 'b' for tunnel geneve offloading. */
>  static inline void
>  dp_packet_hwol_set_tunnel_geneve(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_GENEVE;
>  }
>  
> -/* Mark packet 'b' for tunnel vxlan offloading.  It implies that
> - * the packet 'b' is marked for tunnel vxlan offloading. */
> +/* Mark packet 'b' for tunnel vxlan offloading. */
>  static inline void
>  dp_packet_hwol_set_tunnel_vxlan(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_VXLAN;
>  }
>  
> -/* Mark packet 'b' for out ipv4 packet. */
> +/* Mark packet 'b' as a tunnel packet with IPv4 outer header. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv4(struct dp_packet *b)
>  {
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_OUTER_IPV4;
>  }
>  
> -/* Mark packet 'b' for out ipv4 csum offloading. */
> +/* Mark packet 'b' for IPv4 checksum offload for the outer header. */
>  static inline void
>  dp_packet_hwol_set_tx_outer_ipv4_csum(struct dp_packet *b)
>  {
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e4d95d0a3..fb26825ff 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1157,7 +1157,6 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>          conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
>      }
>  
> -
>      /* Limit configured rss hash functions to only those supported
>       * by the eth device. */
>      conf.rx_adv_conf.rss_conf.rss_hf &= info.flow_type_rss_offloads;
> @@ -2570,8 +2569,8 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf)
>  
>          if (mbuf->ol_flags & (RTE_MBUF_F_TX_TUNNEL_GENEVE |
>              RTE_MBUF_F_TX_TUNNEL_VXLAN)) {
> -            mbuf->tso_segsz  = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> -            mbuf->l4_len - mbuf->outer_l3_len;
> +            mbuf->tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> +                              mbuf->l4_len - mbuf->outer_l3_len;
>          } else {
>              mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
>              mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;

...

> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 35767800c..d513194dc 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -245,8 +245,8 @@ udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>   * encapsulated now. */
>  static void
>  dp_packet_tnl_ol_process(const struct netdev *netdev,
> -                             struct dp_packet *packet,
> -                             const struct ovs_action_push_tnl *data)
> +                         struct dp_packet *packet,
> +                         const struct ovs_action_push_tnl *data)
>  {
>      struct udp_header *udp = NULL;
>      uint8_t opt_len = 0;

...

> @@ -269,10 +269,10 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>          }
>  
>          dp_packet_set_l3_len(packet, (char *) dp_packet_l4(packet) -
> -                              (char *) dp_packet_l3(packet));
> +                                     (char *) dp_packet_l3(packet));
>  
> -        if (!strcmp(netdev_get_type(netdev), "geneve") ||
> -            !strcmp(netdev_get_type(netdev), "vxlan")) {
> +        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE ||
> +            data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
>  
>              if (IP_VER(ip->ip_ihl_ver) == 4) {
>                  dp_packet_hwol_set_tx_ipv4(packet);
> @@ -284,7 +284,7 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>  
>          /* Attention please, tunnel inner l2 len is consist of udp header
>           * len and tunnel header len and inner l2 len. */
> -        if (!strcmp(netdev_get_type(netdev), "geneve")) {
> +        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE) {
>              eth = (struct eth_header *)(data->header);
>              ip = (struct ip_header *)(eth + 1);
>              udp = (struct udp_header *)(ip + 1);
> @@ -292,13 +292,13 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>              opt_len = gnh->opt_len * 4;
>              dp_packet_hwol_set_tunnel_geneve(packet);
>              dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
> -                              (char *) dp_packet_eth(packet) +
> -                              GENEVE_BASE_HLEN + opt_len);
> -        } else if (!strcmp(netdev_get_type(netdev), "vxlan")) {
> +                                         (char *) dp_packet_eth(packet) +
> +                                         GENEVE_BASE_HLEN + opt_len);
> +        } else if (data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
>              dp_packet_hwol_set_tunnel_vxlan(packet);
>              dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
> -                              (char *) dp_packet_eth(packet) +
> -                              VXLAN_HLEN);
> +                                         (char *) dp_packet_eth(packet) +
> +                                         VXLAN_HLEN);
>          }
>      }
>  }

With the changes above in place the netdev parameter of
dp_packet_tnl_ol_process() is no longer used.
As GitHub actions configure OvS compilation with --enable-Werror
this results in a build error [1].

[1] https://github.com/ovsrobot/ovs/actions/runs/7553819191

In order to move things along I have locally squashed the following into
this patch:

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index d513194dc462..5065a40a7fc0 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -244,8 +244,7 @@ udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
 /* Calculate inner l2 l3 l4 len as tunnel outer header is not
  * encapsulated now. */
 static void
-dp_packet_tnl_ol_process(const struct netdev *netdev,
-                         struct dp_packet *packet,
+dp_packet_tnl_ol_process(struct dp_packet *packet,
                          const struct ovs_action_push_tnl *data)
 {
     struct udp_header *udp = NULL;
@@ -304,7 +303,7 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
 }
 
 void
-netdev_tnl_push_udp_header(const struct netdev *netdev,
+netdev_tnl_push_udp_header(const struct netdev *netdev OVS_UNUSED,
                            struct dp_packet *packet,
                            const struct ovs_action_push_tnl *data)
 {
@@ -313,7 +312,7 @@ netdev_tnl_push_udp_header(const struct netdev *netdev,
     uint16_t l3_ofs = packet->l3_ofs;
     uint16_t l4_ofs = packet->l4_ofs;
 
-    dp_packet_tnl_ol_process(netdev, packet, data);
+    dp_packet_tnl_ol_process(packet, data);
     udp = netdev_tnl_push_ip_header(packet, data->header, data->header_len,
                                     &ip_tot_size, 0);
 

And pushed it to my own tree to allow GitHub actions to run [2].
As of writhing those tests are still running, but so far there are no
errors.

[2] https://github.com/horms/ovs/actions/runs/7559436191
Simon Horman Jan. 17, 2024, 5:54 p.m. UTC | #3 
On Wed, Jan 17, 2024 at 05:31:56PM +0000, Simon Horman wrote:
> On Wed, Jan 17, 2024 at 05:07:25PM +0800, Dexia Li via dev wrote:
> > For userspace vxlan and geneve tunnel tso, this commit
> > fix one packet leak and modify style issues.
> > 
> > Signed-off-by: Dexia Li <dexia.li@jaguarmicro.com>

...

> With the changes above in place the netdev parameter of
> dp_packet_tnl_ol_process() is no longer used.
> As GitHub actions configure OvS compilation with --enable-Werror
> this results in a build error [1].
> 
> [1] https://github.com/ovsrobot/ovs/actions/runs/7553819191
> 
> In order to move things along I have locally squashed the following into
> this patch:
> 
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index d513194dc462..5065a40a7fc0 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -244,8 +244,7 @@ udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>  /* Calculate inner l2 l3 l4 len as tunnel outer header is not
>   * encapsulated now. */
>  static void
> -dp_packet_tnl_ol_process(const struct netdev *netdev,
> -                         struct dp_packet *packet,
> +dp_packet_tnl_ol_process(struct dp_packet *packet,
>                           const struct ovs_action_push_tnl *data)
>  {
>      struct udp_header *udp = NULL;
> @@ -304,7 +303,7 @@ dp_packet_tnl_ol_process(const struct netdev *netdev,
>  }
>  
>  void
> -netdev_tnl_push_udp_header(const struct netdev *netdev,
> +netdev_tnl_push_udp_header(const struct netdev *netdev OVS_UNUSED,
>                             struct dp_packet *packet,
>                             const struct ovs_action_push_tnl *data)
>  {
> @@ -313,7 +312,7 @@ netdev_tnl_push_udp_header(const struct netdev *netdev,
>      uint16_t l3_ofs = packet->l3_ofs;
>      uint16_t l4_ofs = packet->l4_ofs;
>  
> -    dp_packet_tnl_ol_process(netdev, packet, data);
> +    dp_packet_tnl_ol_process(packet, data);
>      udp = netdev_tnl_push_ip_header(packet, data->header, data->header_len,
>                                      &ip_tot_size, 0);
>  
> 
> And pushed it to my own tree to allow GitHub actions to run [2].
> As of writhing those tests are still running, but so far there are no
> errors.
> 
> [2] https://github.com/horms/ovs/actions/runs/7559436191

Sorry, I forgot to trim my previous email properly.
And I didn't notice that Ilya had already responded to this patch.
diff mbox series

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index cb20608d7..e7738c37a 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -548,7 +548,7 @@  dp_packet_compare_offsets(struct dp_packet *b1, struct dp_packet *b2,
 
 void
 dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
-                                uint64_t flags)
+                                    uint64_t flags)
 {
     if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
         if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
@@ -558,7 +558,7 @@  dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
         }
     }
 
-    if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
+    if (!dp_packet_hwol_is_outer_udp_cksum(p)) {
         return;
     }
 
@@ -596,7 +596,7 @@  dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
         return;
     }
 
-    if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
+    if (dp_packet_l4_checksum_good(p) && !tnl_inner) {
         dp_packet_hwol_reset_tx_l4_csum(p);
         return;
     }
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index cf341f09c..c9ec47c31 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -92,16 +92,16 @@  enum dp_packet_offload_mask {
     /* Offload packet is tunnel VXLAN. */
     DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
                 RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
-    /* Offload tunnel packet, out is ipv4 */
+    /* Offload tunnel packet, out is IPV4 */
     DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
                 RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
-    /* Offload TUNNEL out ipv4 checksum */
+    /* Offload tunnel out IPV4 checksum */
     DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IP_CKSUM,
                 RTE_MBUF_F_TX_OUTER_IP_CKSUM, 0x10000),
-    /* Offload TUNNEL out udp checksum */
+    /* Offload tunnel out UDP checksum */
     DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_UDP_CKSUM,
                 RTE_MBUF_F_TX_OUTER_UDP_CKSUM, 0x20000),
-    /* Offload tunnel packet, out is ipv6 */
+    /* Offload tunnel packet, out is IPV6 */
     DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV6,
                 RTE_MBUF_F_TX_OUTER_IPV6, 0x40000),
 
@@ -164,9 +164,9 @@  struct dp_packet {
                                     * or UINT16_MAX. */
     uint16_t l4_ofs;               /* Transport-level header offset,
                                       or UINT16_MAX. */
-    uint16_t inner_l3_ofs;         /* inner Network-level header offset,
+    uint16_t inner_l3_ofs;         /* Inner Network-level header offset,
                                     * or UINT16_MAX. */
-    uint16_t inner_l4_ofs;         /* inner Transport-level header offset,
+    uint16_t inner_l4_ofs;         /* Inner Transport-level header offset,
                                       or UINT16_MAX. */
     uint32_t cutlen;               /* length in bytes to cut from the end. */
     ovs_be32 packet_type;          /* Packet type as defined in OpenFlow */
@@ -279,8 +279,8 @@  bool dp_packet_compare_offsets(struct dp_packet *good,
                                struct dp_packet *test,
                                struct ds *err_str);
 void dp_packet_ol_send_prepare(struct dp_packet *, uint64_t);
-void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
-                                         uint64_t flags);
+void dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *,
+                                         uint64_t);
 
 
 
@@ -640,19 +640,19 @@  dp_packet_flow_mark_ptr(const struct dp_packet *b)
 static inline void
 dp_packet_set_l2_len(struct dp_packet *b OVS_UNUSED, size_t l2_len OVS_UNUSED)
 {
-    /* There are no implementation */
+    /* There is no implementation. */
 }
 
 static inline void
 dp_packet_set_l3_len(struct dp_packet *b OVS_UNUSED, size_t l3_len OVS_UNUSED)
 {
-    /* There are no implementation */
+    /* There is no implementation. */
 }
 
 static inline void
 dp_packet_set_l4_len(struct dp_packet *b OVS_UNUSED, size_t l4_len OVS_UNUSED)
 {
-    /* There are no implementation */
+    /* There is no implementation. */
 }
 
 static inline uint32_t *
@@ -1162,23 +1162,26 @@  dp_packet_hwol_is_tunnel_vxlan(struct dp_packet *b)
     return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_TUNNEL_VXLAN);
 }
 
-/* Returns 'true' if packet 'b' is marked for out ipv4. */
+/* Returns 'true' if packet 'b' is marked as a tunnel
+ * packet with outer header being IPv4. */
 static inline bool
 dp_packet_hwol_is_outer_ipv4(struct dp_packet *b)
 {
     return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IPV4);
 }
 
-/* Returns 'true' if packet 'b' is marked for out ipv4 csum offload. */
+/* Returns 'true' if a tunnel packet 'b' is marked for IPv4 checksum offload
+ * for the outer header. */
 static inline bool
 dp_packet_hwol_is_outer_ipv4_cksum(struct dp_packet *b)
 {
     return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_IP_CKSUM);
 }
 
-/* Returns 'true' if packet 'b' is marked for out udp csum offload. */
+/* Returns 'true' if a tunnel packet 'b' is marked for UDP checksum  offload
+ * for the outer header. */
 static inline bool
-dp_packet_hwol_is_outer_UDP_cksum(struct dp_packet *b)
+dp_packet_hwol_is_outer_udp_cksum(struct dp_packet *b)
 {
     return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_OUTER_UDP_CKSUM);
 }
@@ -1205,7 +1208,8 @@  dp_packet_hwol_set_tx_ipv6(struct dp_packet *a)
     *dp_packet_ol_flags_ptr(a) |= DP_PACKET_OL_TX_IPV6;
 }
 
-/* Mark packet 'a' as IPv6. */
+/* Returns 'true' if packet 'b' is marked as a tunnel
+ * packet with outer header being IPv6. */
 static inline void
 dp_packet_hwol_set_tx_outer_ipv6(struct dp_packet *a)
 {
@@ -1266,30 +1270,28 @@  dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
     *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
 }
 
-/* Mark packet 'b' for tunnel geneve offloading.  It implies that
- * the packet 'b' is marked for tunnel geneve offloading. */
+/* Mark packet 'b' for tunnel geneve offloading. */
 static inline void
 dp_packet_hwol_set_tunnel_geneve(struct dp_packet *b)
 {
     *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_GENEVE;
 }
 
-/* Mark packet 'b' for tunnel vxlan offloading.  It implies that
- * the packet 'b' is marked for tunnel vxlan offloading. */
+/* Mark packet 'b' for tunnel vxlan offloading. */
 static inline void
 dp_packet_hwol_set_tunnel_vxlan(struct dp_packet *b)
 {
     *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TUNNEL_VXLAN;
 }
 
-/* Mark packet 'b' for out ipv4 packet. */
+/* Mark packet 'b' as a tunnel packet with IPv4 outer header. */
 static inline void
 dp_packet_hwol_set_tx_outer_ipv4(struct dp_packet *b)
 {
     *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_OUTER_IPV4;
 }
 
-/* Mark packet 'b' for out ipv4 csum offloading. */
+/* Mark packet 'b' for IPv4 checksum offload for the outer header. */
 static inline void
 dp_packet_hwol_set_tx_outer_ipv4_csum(struct dp_packet *b)
 {
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e4d95d0a3..fb26825ff 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1157,7 +1157,6 @@  dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
         conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
     }
 
-
     /* Limit configured rss hash functions to only those supported
      * by the eth device. */
     conf.rx_adv_conf.rss_conf.rss_hf &= info.flow_type_rss_offloads;
@@ -2570,8 +2569,8 @@  netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf)
 
         if (mbuf->ol_flags & (RTE_MBUF_F_TX_TUNNEL_GENEVE |
             RTE_MBUF_F_TX_TUNNEL_VXLAN)) {
-            mbuf->tso_segsz  = dev->mtu - mbuf->l2_len - mbuf->l3_len -
-            mbuf->l4_len - mbuf->outer_l3_len;
+            mbuf->tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len -
+                              mbuf->l4_len - mbuf->outer_l3_len;
         } else {
             mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
             mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index 35767800c..d513194dc 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -245,8 +245,8 @@  udp_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
  * encapsulated now. */
 static void
 dp_packet_tnl_ol_process(const struct netdev *netdev,
-                             struct dp_packet *packet,
-                             const struct ovs_action_push_tnl *data)
+                         struct dp_packet *packet,
+                         const struct ovs_action_push_tnl *data)
 {
     struct udp_header *udp = NULL;
     uint8_t opt_len = 0;
@@ -256,8 +256,8 @@  dp_packet_tnl_ol_process(const struct netdev *netdev,
 
     /* l2 l3 l4 len refer to inner len, tunnel outer
      * header is not encapsulated here. */
-   if (dp_packet_hwol_l4_mask(packet)) {
-       ip = dp_packet_l3(packet);
+    if (dp_packet_hwol_l4_mask(packet)) {
+        ip = dp_packet_l3(packet);
 
         if (ip->ip_proto == IPPROTO_TCP) {
             struct tcp_header *th = dp_packet_l4(packet);
@@ -269,10 +269,10 @@  dp_packet_tnl_ol_process(const struct netdev *netdev,
         }
 
         dp_packet_set_l3_len(packet, (char *) dp_packet_l4(packet) -
-                              (char *) dp_packet_l3(packet));
+                                     (char *) dp_packet_l3(packet));
 
-        if (!strcmp(netdev_get_type(netdev), "geneve") ||
-            !strcmp(netdev_get_type(netdev), "vxlan")) {
+        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE ||
+            data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
 
             if (IP_VER(ip->ip_ihl_ver) == 4) {
                 dp_packet_hwol_set_tx_ipv4(packet);
@@ -284,7 +284,7 @@  dp_packet_tnl_ol_process(const struct netdev *netdev,
 
         /* Attention please, tunnel inner l2 len is consist of udp header
          * len and tunnel header len and inner l2 len. */
-        if (!strcmp(netdev_get_type(netdev), "geneve")) {
+        if (data->tnl_type == OVS_VPORT_TYPE_GENEVE) {
             eth = (struct eth_header *)(data->header);
             ip = (struct ip_header *)(eth + 1);
             udp = (struct udp_header *)(ip + 1);
@@ -292,13 +292,13 @@  dp_packet_tnl_ol_process(const struct netdev *netdev,
             opt_len = gnh->opt_len * 4;
             dp_packet_hwol_set_tunnel_geneve(packet);
             dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
-                              (char *) dp_packet_eth(packet) +
-                              GENEVE_BASE_HLEN + opt_len);
-        } else if (!strcmp(netdev_get_type(netdev), "vxlan")) {
+                                         (char *) dp_packet_eth(packet) +
+                                         GENEVE_BASE_HLEN + opt_len);
+        } else if (data->tnl_type == OVS_VPORT_TYPE_VXLAN) {
             dp_packet_hwol_set_tunnel_vxlan(packet);
             dp_packet_set_l2_len(packet, (char *) dp_packet_l3(packet) -
-                              (char *) dp_packet_eth(packet) +
-                              VXLAN_HLEN);
+                                         (char *) dp_packet_eth(packet) +
+                                         VXLAN_HLEN);
         }
     }
 }
diff --git a/lib/netdev.c b/lib/netdev.c
index db0610304..dc3089396 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -70,6 +70,8 @@  COVERAGE_DEFINE(netdev_sent);
 COVERAGE_DEFINE(netdev_add_router);
 COVERAGE_DEFINE(netdev_get_stats);
 COVERAGE_DEFINE(netdev_push_header_drops);
+COVERAGE_DEFINE(netdev_vxlan_tso_not_support);
+COVERAGE_DEFINE(netdev_geneve_tso_not_support);
 COVERAGE_DEFINE(netdev_soft_seg_good);
 COVERAGE_DEFINE(netdev_soft_seg_drops);
 
@@ -915,12 +917,16 @@  netdev_send(struct netdev *netdev, int qid, struct dp_packet_batch *batch,
                 if (dp_packet_hwol_is_tunnel_vxlan(packet)
                     && !(netdev_flags & NETDEV_TX_VXLAN_TNL_TSO)) {
                         VLOG_ERR("No VXLAN TSO support");
+                        COVERAGE_INC(netdev_vxlan_tso_not_support);
+                        dp_packet_delete(packet);
                         return false;
                 }
 
                 if (dp_packet_hwol_is_tunnel_geneve(packet)
                     && !(netdev_flags & NETDEV_TX_GENEVE_TNL_TSO)) {
                         VLOG_ERR("No GENEVE TSO support");
+                        COVERAGE_INC(netdev_geneve_tso_not_support);
+                        dp_packet_delete(packet);
                         return false;
                 }
                 return netdev_send_tso(netdev, qid, batch, concurrent_txq);
@@ -1001,19 +1007,29 @@  netdev_push_header(const struct netdev *netdev,
     size_t i, size = dp_packet_batch_size(batch);
 
     DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
-        if (OVS_UNLIKELY(strcmp(netdev_get_type(netdev), "vxlan") &&
-            strcmp(netdev_get_type(netdev), "geneve") &&
+        if (OVS_UNLIKELY(data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
+            data->tnl_type != OVS_VPORT_TYPE_VXLAN &&
             dp_packet_hwol_is_tso(packet))) {
             COVERAGE_INC(netdev_push_header_drops);
             dp_packet_delete(packet);
-            VLOG_WARN_RL(&rl, "%s: Tunneling packets with tso HW offload"
-                     "flags is not supported: packet dropped",
+            VLOG_WARN_RL(&rl, "Tunneling packets with TSO is not supported"
+                     "for %s tunnels, packet dropped",
                      netdev_get_name(netdev));
         } else {
-            if (strcmp(netdev_get_type(netdev), "vxlan") &&
-                strcmp(netdev_get_type(netdev), "geneve")) {
+            if (OVS_UNLIKELY(dp_packet_hwol_is_tunnel_geneve(packet) ||
+                dp_packet_hwol_is_tunnel_vxlan(packet))) {
+                COVERAGE_INC(netdev_push_header_drops);
+                dp_packet_delete(packet);
+                VLOG_WARN_RL(&rl, "%s tunnel tso packet before encap"
+                         "not support, packet dropped",
+                         netdev_get_name(netdev));
+            }
+
+            if (data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
+                data->tnl_type != OVS_VPORT_TYPE_VXLAN) {
                 dp_packet_ol_send_prepare(packet, 0);
             }
+
             netdev->netdev_class->push_header(netdev, packet, data);
 
             pkt_metadata_init(&packet->md, data->out_port);