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 <[email protected]>
> ---
> 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);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
