On Wed, Jul 13, 2022 at 8:45 PM Mike Pattrick <[email protected]> wrote:
>
> From: Flavio Leitner <[email protected]>
>
> The netdev receiving packets is supposed to provide the flags
> indicating if the IP csum was verified and it is GOOD or BAD,
> otherwise the stack will check when appropriate by software.
>
> If the packet comes with good checksum, then postpone the
> checksum calculation to the egress device if needed.
>
> When encapsulate a packet with that flag, set the checksum
> of the inner IP header since that is not yet supported.
>
> Calculate the IP csum when the packet is going to be sent over
> a device that doesn't support the feature.
>
> Linux devices don't support IP csum offload alone, so the
> support is not enabled.
>
> Signed-off-by: Flavio Leitner <[email protected]>
> Co-authored-by: Mike Pattrick <[email protected]>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> v5:
> - Refactor was mostly removed, except for valid->good
> - Reset unsupported offload flags in send_prepare
> - Moved send_prepare from process_upcall to netdev_upcall
>
> v6:
> - Re-added tests that were incorrectly excluded from v5
[snip]
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 081900576..811c62a87 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -397,8 +397,9 @@ enum dpdk_hw_ol_features {
> NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0,
> NETDEV_RX_HW_CRC_STRIP = 1 << 1,
> NETDEV_RX_HW_SCATTER = 1 << 2,
> - NETDEV_TX_TSO_OFFLOAD = 1 << 3,
> - NETDEV_TX_SCTP_CHECKSUM_OFFLOAD = 1 << 4,
> + NETDEV_TX_IPV4_CKSUM_OFFLOAD = 1 << 3,
> + NETDEV_TX_TSO_OFFLOAD = 1 << 4,
> + NETDEV_TX_SCTP_CHECKSUM_OFFLOAD = 1 << 5,
> };
>
> /*
> @@ -984,6 +985,10 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int
> n_rxq, int n_txq)
> conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_KEEP_CRC;
> }
>
> + if (dev->hw_ol_features & NETDEV_TX_IPV4_CKSUM_OFFLOAD) {
> + conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
> + }
> +
> if (dev->hw_ol_features & NETDEV_TX_TSO_OFFLOAD) {
> conf.txmode.offloads |= DPDK_TX_TSO_OFFLOAD_FLAGS;
> if (dev->hw_ol_features & NETDEV_TX_SCTP_CHECKSUM_OFFLOAD) {
> @@ -1124,6 +1129,12 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
> }
>
> + if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM) {
> + dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
> + } else {
> + dev->hw_ol_features &= ~NETDEV_TX_IPV4_CKSUM_OFFLOAD;
> + }
> +
> dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
> if (userspace_tso_enabled()) {
> if ((info.tx_offload_capa & tx_tso_offload_capa)
> @@ -1693,16 +1704,12 @@ netdev_dpdk_get_config(const struct netdev *netdev,
> struct smap *args)
> dev->requested_txq_size);
> smap_add_format(args, "configured_txq_descriptors", "%d",
> dev->txq_size);
> - if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
> - smap_add(args, "rx_csum_offload", "true");
> - } else {
> - smap_add(args, "rx_csum_offload", "false");
> - }
> - if (dev->hw_ol_features & NETDEV_TX_TSO_OFFLOAD) {
> - smap_add(args, "tx_tso_offload", "true");
> - } else {
> - smap_add(args, "tx_tso_offload", "false");
> - }
> +#define HWOL_SMAP_ADD(FIELD, FLAG) \
> + smap_add(args, FIELD, dev->hw_ol_features & FLAG ? "true" : "false");
> + HWOL_SMAP_ADD("rx_csum_offload", NETDEV_RX_CHECKSUM_OFFLOAD);
> + HWOL_SMAP_ADD("tx_ip_csum_offload", NETDEV_TX_IPV4_CKSUM_OFFLOAD);
> + HWOL_SMAP_ADD("tx_tso_offload", NETDEV_TX_TSO_OFFLOAD);
> +#undef HWOL_SMAP_ADD
> smap_add(args, "lsc_interrupt_mode",
> dev->lsc_interrupt_mode ? "true" : "false");
>
> @@ -2145,12 +2152,10 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
> struct rte_mbuf *mbuf)
> {
> struct dp_packet *pkt = CONTAINER_OF(mbuf, struct dp_packet, mbuf);
>
> - if (mbuf->ol_flags & RTE_MBUF_F_TX_L4_MASK) {
> - mbuf->l2_len = (char *)dp_packet_l3(pkt) - (char
> *)dp_packet_eth(pkt);
> - mbuf->l3_len = (char *)dp_packet_l4(pkt) - (char *)dp_packet_l3(pkt);
> - mbuf->outer_l2_len = 0;
> - mbuf->outer_l3_len = 0;
> - }
> + mbuf->l2_len = (char *) dp_packet_l3(pkt) - (char *) dp_packet_eth(pkt);
> + mbuf->l3_len = (char *) dp_packet_l4(pkt) - (char *) dp_packet_l3(pkt);
> + mbuf->outer_l2_len = 0;
> + mbuf->outer_l3_len = 0;
>
> if (mbuf->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
> struct tcp_header *th = dp_packet_l4(pkt);
> @@ -2210,13 +2215,11 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int
> qid,
> uint32_t nb_tx = 0;
> uint16_t nb_tx_prep = cnt;
>
> - if (userspace_tso_enabled()) {
> - nb_tx_prep = rte_eth_tx_prepare(dev->port_id, qid, pkts, cnt);
> - if (nb_tx_prep != cnt) {
> - VLOG_WARN_RL(&rl, "%s: Output batch contains invalid packets. "
> - "Only %u/%u are valid: %s", dev->up.name,
> nb_tx_prep,
> - cnt, rte_strerror(rte_errno));
> - }
> + nb_tx_prep = rte_eth_tx_prepare(dev->port_id, qid, pkts, cnt);
> + if (nb_tx_prep != cnt) {
> + VLOG_WARN_RL(&rl, "%s: Output batch contains invalid packets. "
> + "Only %u/%u are valid: %s", dev->up.name, nb_tx_prep,
> + cnt, rte_strerror(rte_errno));
> }
>
> while (nb_tx != nb_tx_prep) {
> @@ -2656,12 +2659,10 @@ dpdk_copy_dp_packet_to_mbuf(struct rte_mempool *mp,
> struct dp_packet *pkt_orig)
> memcpy(&pkt_dest->l2_pad_size, &pkt_orig->l2_pad_size,
> sizeof(struct dp_packet) - offsetof(struct dp_packet,
> l2_pad_size));
>
> - if (mbuf_dest->ol_flags & RTE_MBUF_F_TX_L4_MASK) {
> - mbuf_dest->l2_len = (char *)dp_packet_l3(pkt_dest)
> - - (char *)dp_packet_eth(pkt_dest);
> - mbuf_dest->l3_len = (char *)dp_packet_l4(pkt_dest)
> - - (char *) dp_packet_l3(pkt_dest);
> - }
> + mbuf_dest->l2_len = (char *) dp_packet_l3(pkt_dest)
> + - (char *) dp_packet_eth(pkt_dest);
> + mbuf_dest->l3_len = (char *) dp_packet_l4(pkt_dest)
> + - (char *) dp_packet_l3(pkt_dest);
>
> return pkt_dest;
> }
Stopping at this patch, there was one issue in the code, that left me
perplex for while :-).
Running with userspace-tso-enable="false" and sending 2 packets for
which a OF rule changes the dest ip address:
(gdb) b rte_eth_tx_burst
Breakpoint 3 at 0x936600: rte_eth_tx_burst. (38 locations)
(gdb) commands
Type commands for breakpoint(s) 3, one per line.
End with a line saying just "end".
>p tx_pkts[0].ol_flags
>p tx_pkts[0].l2_len
>x/hx (uint16_t *)(tx_pkts[0].buf_addr + tx_pkts[0].data_off + 24)
>continue
>end
- First packet:
Thread 15 "pmd-c05/id:11" hit Breakpoint 3, rte_eth_tx_burst
(nb_pkts=<optimized out>, tx_pkts=0x7f09b4005970, queue_id=3,
port_id=3) at /root/ovs/dpdk-dir/v21.11/install/include/rte_ethdev.h:5680
5680 nb_pkts = p->tx_pkt_burst(qd, tx_pkts, nb_pkts);
$9 = 36028797018964098
$10 = 0
0x1590f15158: 0x82ca
Which translates to:
RTE_MBUF_F_RX_RSS_HASH
RTE_MBUF_F_RX_IP_CKSUM_GOOD
RTE_MBUF_F_TX_IPV4
The packet checksum in memory is correct, because it got resolved in
sw, during the "upcall" handling.
OVS does not ask for hw ip csum, which is expected too.
All is good, the packet is received:
07:14:12.982762 0c:42:a1:00:a8:7c > 0c:42:a1:00:a8:7d, ethertype IPv4
(0x0800), length 60: (tos 0x0, ttl 64, id 1, offset 0, flags [none],
proto unknown (254), length 20)
...
0x0010: 0014 0001 0000 40fe *ca82* c0a8 1701 c0a8 ......@.........
- Second packet:
Thread 15 "pmd-c05/id:11" hit Breakpoint 3, rte_eth_tx_burst
(nb_pkts=<optimized out>, tx_pkts=0x7f09b4005970, queue_id=3,
port_id=3) at /root/ovs/dpdk-dir/v21.11/install/include/rte_ethdev.h:5680
5680 nb_pkts = p->tx_pkt_burst(qd, tx_pkts, nb_pkts);
$11 = 54043195528445954
$12 = 0
0x1590f15d18: 0x97ca
Which translates to:
RTE_MBUF_F_RX_RSS_HASH
RTE_MBUF_F_TX_IP_CKSUM
RTE_MBUF_F_TX_IPV4
The packet checksum is invalid in memory.
OVS asks for TX ip checksum.
But l2_len is empty.
This is because of a check in netdev_dpdk_common_send()
(userspace_tso_enabled() == false), and as a consequence
netdev_dpdk_prep_hwol_batch() is not called.
But.. testing with a mlx5 hw/driver, the received packet has a valid csum:
07:14:12.984818 0c:42:a1:00:a8:7c > 0c:42:a1:00:a8:7d, ethertype IPv4
(0x0800), length 60: (tos 0x0, ttl 64, id 1, offset 0, flags [none],
proto unknown (254), length 20)
...
0x0010: 0014 0001 0000 40fe *ca82* c0a8 1701 c0a8 ......@.........
My assumption is that the mlx5 hw does not need l2_len to determine
where to find the ip header.
This might be different with other nics and the mbuf API wrt IP
checksum is still explicit:
https://git.dpdk.org/dpdk/tree/lib/mbuf/rte_mbuf_core.h?h=v21.11#n411
/**
* Offload the IP checksum in the hardware. The flag RTE_MBUF_F_TX_IPV4 should
* also be set by the application, although a PMD will only check
* RTE_MBUF_F_TX_IP_CKSUM.
* - fill the mbuf offload information: l2_len, l3_len
*/
#define RTE_MBUF_F_TX_IP_CKSUM (1ULL << 54)
So netdev_dpdk_prep_hwol_batch should be called regardless of
userspace_tso_enabled().
Diffing v4 with v6, it seems this is a rebase issue:
@@ -2770,9 +2770,11 @@ netdev_dpdk_common_send(struct netdev *netdev,
struct dp_packet_batch *batch,
pkt_cnt = cnt;
/* Prepare each mbuf for hardware offloading. */
- cnt = netdev_dpdk_prep_ol_batch(dev, pkts, pkt_cnt);
- stats->tx_invalid_ol_drops += pkt_cnt - cnt;
- pkt_cnt = cnt;
+ if (userspace_tso_enabled()) {
+ cnt = netdev_dpdk_prep_hwol_batch(dev, pkts, pkt_cnt);
+ stats->tx_invalid_hwol_drops += pkt_cnt - cnt;
+ pkt_cnt = cnt;
+ }
/* Apply Quality of Service policy. */
cnt = netdev_dpdk_qos_run(dev, pkts, pkt_cnt, true);
The rest lgtm.
With this fixed, you can add my:
Reviewed-by: David Marchand <[email protected]>
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
