On 5/17/23 05:11, Mike Pattrick wrote:
> From: Flavio Leitner <[email protected]>
>
> The netdev receiving packets is supposed to provide the flags
> indicating if the IP checksum 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 checksum when the packet is going to be sent over
> a device that doesn't support the feature.
>
> Linux devices don't support IP checksum 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]>
> --
> Since v9:
> - Removed duplicative field tx_ip_csum_offload from netdev-dpdk.c
> - Left rx_csum_offload field as it is not duplicative
> - Moved system-userspace-offload.at tests to dpif-netdev.at
> - Various visual changes
> - Extended miniflow_extract changes into avx512 code
> Since v10:
> - avx512 checksum length corrected
> Since v11:
> - If hw-offload and userspace-tso is enabled, don't allow dpdk to
> offload RAW_ENCAP
>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> lib/conntrack.c | 19 ++++----
> lib/dp-packet.c | 15 ++++++
> lib/dp-packet.h | 62 +++++++++++++++++++++++--
> lib/dpif-netdev-extract-avx512.c | 5 ++
> lib/dpif-netdev.c | 2 +
> lib/flow.c | 15 ++++--
> lib/ipf.c | 11 +++--
> lib/netdev-dpdk.c | 71 +++++++++++++++++++----------
> lib/netdev-dummy.c | 23 ++++++++++
> lib/netdev-native-tnl.c | 21 ++++++---
> lib/netdev-offload-dpdk.c | 18 ++++++--
> lib/netdev.c | 16 +++++++
> lib/odp-execute-avx512.c | 19 +++++---
> lib/odp-execute.c | 21 +++++++--
> lib/packets.c | 34 +++++++++++---
> tests/dpif-netdev.at | 78 ++++++++++++++++++++++++++++++++
> 16 files changed, 359 insertions(+), 71 deletions(-)
>
<snip>
> @@ -1174,6 +1190,13 @@ netdev_dummy_send(struct netdev *netdev, int qid,
> }
>
> ovs_mutex_lock(&dev->mutex);
> + if (dp_packet_hwol_tx_ip_csum(packet)) {
> + if (!dp_packet_ip_checksum_good(packet)) {
There is unnecessary level of nesting.
> + dp_packet_ip_set_header_csum(packet);
> + dp_packet_ol_set_ip_csum_good(packet);
> + }
> + }
> +
> dev->stats.tx_packets++;
> dev->txq_stats[qid].packets++;
> dev->stats.tx_bytes += size;
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 9abdf5107..bd64a05c9 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -88,7 +88,10 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet,
> struct flow_tnl *tnl,
>
> ovs_be32 ip_src, ip_dst;
>
> - if (OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) {
> + /* A packet coming from a network device might have the
> + * csum already checked. In this case, skip the check. */
> + if (OVS_UNLIKELY(!dp_packet_ip_checksum_good(packet))
> + && !dp_packet_hwol_tx_ip_csum(packet)) {
> if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> return NULL;
> @@ -142,7 +145,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet,
> struct flow_tnl *tnl,
> *
> * This function sets the IP header's ip_tot_len field (which should be
> zeroed
> * as part of 'header') and puts its value into '*ip_tot_size' as well. Also
> - * updates IP header checksum, as well as the l3 and l4 offsets in 'packet'.
> + * updates IP header checksum if not offloaded, as well as the l3 and l4
> + * offsets in the 'packet'.
> *
> * Return pointer to the L4 header added to 'packet'. */
> void *
> @@ -167,11 +171,16 @@ netdev_tnl_push_ip_header(struct dp_packet *packet,
> *ip_tot_size -= IPV6_HEADER_LEN;
> ip6->ip6_plen = htons(*ip_tot_size);
> packet->l4_ofs = dp_packet_size(packet) - *ip_tot_size;
> + dp_packet_hwol_set_tx_ipv6(packet);
> + dp_packet_ol_reset_ip_csum_good(packet);
> return ip6 + 1;
> } else {
> ip = netdev_tnl_ip_hdr(eth);
> ip->ip_tot_len = htons(*ip_tot_size);
> - ip->ip_csum = recalc_csum16(ip->ip_csum, 0, ip->ip_tot_len);
> + /* Postpone checksum to when the packet is pushed to the port. */
> + dp_packet_hwol_set_tx_ipv4(packet);
> + dp_packet_hwol_set_tx_ip_csum(packet);
> + dp_packet_ol_reset_ip_csum_good(packet);
> *ip_tot_size -= IP_HEADER_LEN;
> packet->l4_ofs = dp_packet_size(packet) - *ip_tot_size;
> return ip + 1;
> @@ -190,7 +199,7 @@ udp_extract_tnl_md(struct dp_packet *packet, struct
> flow_tnl *tnl,
> }
>
> if (udp->udp_csum) {
> - if (OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
> + if (OVS_UNLIKELY(!dp_packet_l4_checksum_good(packet))) {
> uint32_t csum;
> if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> @@ -297,8 +306,8 @@ netdev_tnl_ip_build_header(struct ovs_action_push_tnl
> *data,
> ip->ip_frag_off = (params->flow->tunnel.flags &
> FLOW_TNL_F_DONT_FRAGMENT) ?
> htons(IP_DF) : 0;
>
> - /* Checksum has already been zeroed by eth_build_header. */
> - ip->ip_csum = csum(ip, sizeof *ip);
> + /* The checksum will be calculated when the headers are pushed
> + * to the packet if offloading is not enabled. */
>
> data->header_len += IP_HEADER_LEN;
> return ip + 1;
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index b3421c099..51ea72e66 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -33,6 +33,7 @@
> #include "openvswitch/vlog.h"
> #include "ovs-rcu.h"
> #include "packets.h"
> +#include "userspace-tso.h"
> #include "uuid.h"
>
> VLOG_DEFINE_THIS_MODULE(netdev_offload_dpdk);
> @@ -2066,7 +2067,7 @@ parse_vlan_push_action(struct flow_actions *actions,
> return 0;
> }
>
> -static void
> +static int
> add_tunnel_push_action(struct flow_actions *actions,
> const struct ovs_action_push_tnl *tnl_push)
> {
> @@ -2074,7 +2075,11 @@ add_tunnel_push_action(struct flow_actions *actions,
>
> if (tnl_push->tnl_type == OVS_VPORT_TYPE_VXLAN &&
> !add_vxlan_encap_action(actions, tnl_push->header)) {
> - return;
> + return 0;
> + }
> +
> + if (userspace_tso_enabled()) {
> + return -1;
Uhm. This doesn't actually work. Does it?
userspace_tso_enabled() doesn't protect us from incorrect checksums as we're
enabling checksum offlaoding by default as they are never actually calculated
while building the tunnel header. Or am I missing something?
OTOH, I digged into implementation of the RAW_ENCAP, and it's not really that
raw as it documented. It's currently supported only by nfp and mlx drivers
and both are using internal protocol specific offloading mechanisms. They
parse the provided "raw" headers and either not use the checksum fields, or
zero them out for internal purposes as the HW will re-calculate them anyway.
So, I'd say we drop that part for now and allow offloading. The RAW_ENCAP
API is experimantal and needs some documentaion clarification regarding
header checksums.
> }
>
> raw_encap = xzalloc(sizeof *raw_encap);
> @@ -2083,6 +2088,7 @@ add_tunnel_push_action(struct flow_actions *actions,
> raw_encap->size = tnl_push->header_len;
>
> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RAW_ENCAP, raw_encap);
> + return 0;
> }
>
> static int
> @@ -2099,7 +2105,9 @@ parse_clone_actions(struct netdev *netdev,
>
> if (clone_type == OVS_ACTION_ATTR_TUNNEL_PUSH) {
> const struct ovs_action_push_tnl *tnl_push = nl_attr_get(ca);
> - add_tunnel_push_action(actions, tnl_push);
> + if (add_tunnel_push_action(actions, tnl_push)) {
> + return -1;
> + }
> } else if (clone_type == OVS_ACTION_ATTR_OUTPUT) {
> if (add_output_action(netdev, actions, ca)) {
> return -1;
> @@ -2205,7 +2213,9 @@ parse_flow_actions(struct netdev *netdev,
> } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_TUNNEL_PUSH) {
> const struct ovs_action_push_tnl *tnl_push = nl_attr_get(nla);
>
> - add_tunnel_push_action(actions, tnl_push);
> + if (add_tunnel_push_action(actions, tnl_push)) {
> + return -1;
> + }
> } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CLONE &&
> left <= NLA_ALIGN(nla->nla_len)) {
> const struct nlattr *clone_actions = nl_attr_get(nla);
> diff --git a/lib/netdev.c b/lib/netdev.c
> index cc03308fb..5cb4f6f52 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -808,6 +808,14 @@ netdev_send_prepare_packet(const uint64_t netdev_flags,
> return false;
> }
>
> + /* Packet with IP csum offloading enabled was received with verified
> csum.
> + * Leave the IP csum offloading enabled even with good checksum to the
> + * netdev to decide what would be the best to do.
> + * Provide a software fallback in case the device doesn't support IP csum
> + * offloading. Note: Encapsulated packet must have the inner IP header
> + * csum already calculated. */
> + dp_packet_ol_send_prepare(packet, netdev_flags);
> +
> l4_mask = dp_packet_hwol_l4_mask(packet);
> if (l4_mask) {
> if (dp_packet_hwol_l4_is_tcp(packet)) {
> @@ -975,7 +983,15 @@ netdev_push_header(const struct netdev *netdev,
> "not supported: packet dropped",
> netdev_get_name(netdev));
> } else {
> + /* The packet is going to be encapsulated and there is
> + * no support yet for inner network header csum offloading. */
> + if (dp_packet_hwol_tx_ip_csum(packet)
> + && !dp_packet_ip_checksum_good(packet)) {
> + dp_packet_ip_set_header_csum(packet);
> + }
> +
> netdev->netdev_class->push_header(netdev, packet, data);
> +
> pkt_metadata_init(&packet->md, data->out_port);
> dp_packet_batch_refill(batch, packet, i);
> }
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index c28461ec1..93b6b6ccc 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -450,7 +450,6 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch
> *batch,
>
> DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> struct ip_header *nh = dp_packet_l3(packet);
> - ovs_be16 old_csum = ~nh->ip_csum;
>
> /* Load the 20 bytes of the IPv4 header. Without options, which is
> the
> * most common case it's 20 bytes, but can be up to 60 bytes. */
> @@ -463,13 +462,19 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch
> *batch,
> * (v_pkt_masked). */
> __m256i v_new_hdr = _mm256_or_si256(v_key_shuf, v_pkt_masked);
>
> - /* Update the IP checksum based on updated IP values. */
> - uint16_t delta = avx512_ipv4_hdr_csum_delta(v_packet, v_new_hdr);
> - uint32_t new_csum = old_csum + delta;
> - delta = csum_finish(new_csum);
> + if (dp_packet_hwol_tx_ip_csum(packet)) {
> + dp_packet_ol_reset_ip_csum_good(packet);
> + } else {
> + ovs_be16 old_csum = ~nh->ip_csum;
>
> - /* Insert new checksum. */
> - v_new_hdr = _mm256_insert_epi16(v_new_hdr, delta, 5);
> + /* Update the IP checksum based on updated IP values. */
> + uint16_t delta = avx512_ipv4_hdr_csum_delta(v_packet, v_new_hdr);
> + uint32_t new_csum = old_csum + delta;
An empty line here would be nice.
> + delta = csum_finish(new_csum);
> +
> + /* Insert new checksum. */
> + v_new_hdr = _mm256_insert_epi16(v_new_hdr, delta, 5);
> + }
>
> /* If ip_src or ip_dst has been modified, L4 checksum needs to
> * be updated too. */
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
