On Fri, Apr 11, 2025 at 7:36 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 4/9/25 8:16 PM, Mike Pattrick wrote:
> > On Thu, Mar 13, 2025 at 8:44 AM David Marchand
> > <david.march...@redhat.com> wrote:
> >>
> >> As the virtio-net offload API is used for netdev-linux ports, but
> >> provides no information about the potentially encapsulated protocol
> >> concerned by a checksum request, specific informations from this netdev-
> >> specific implementation is propagated into OVS code, and must be
> >> carefully evaluated when some tunnel gets decapsulated.
> >>
> >> This induces a cost in "normal" processing path, while the netdev-linux
> >> path is not performance critical.
> >>
> >> This patch removes such specific information, yet try harder to parse
> >> the packet on the Rx side and set offload flags accordingly for non
> >> encapsulated traffic and for encapsulated traffic, fixes the inner
> >> checksum.
> >>
> >> Signed-off-by: David Marchand <david.march...@redhat.com>
> >> ---
> >> Changes since v3:
> >> - dropped setting Tx flags,
> >>
> >> Changes since v1:
> >> - fixed patch reordering issue: offloads fields was being added instead
> >> of later in this series,
> >>
> >> ---
> >> lib/dp-packet.c | 3 -
> >> lib/dp-packet.h | 24 -------
> >> lib/dpif-netdev-extract-avx512.c | 2 -
> >> lib/flow.c | 6 --
> >> lib/netdev-linux.c | 113 ++++++++++++++++++++++++++++---
> >> lib/netdev-native-tnl.c | 6 +-
> >> 6 files changed, 104 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> >> index dad0d7be3a..6a9bfd63ba 100644
> >> --- a/lib/dp-packet.c
> >> +++ b/lib/dp-packet.c
> >> @@ -39,9 +39,6 @@ dp_packet_init__(struct dp_packet *b, size_t allocated,
> >> enum dp_packet_source so
> >> dp_packet_init_specific(b);
> >> /* By default assume the packet type to be Ethernet. */
> >> b->packet_type = htonl(PT_ETH);
> >> - /* Reset csum start and offset. */
> >> - b->csum_start = 0;
> >> - b->csum_offset = 0;
> >> }
> >>
> >> static void
> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> >> index 549802167d..084e89bd99 100644
> >> --- a/lib/dp-packet.h
> >> +++ b/lib/dp-packet.h
> >> @@ -178,8 +178,6 @@ struct dp_packet {
> >> or UINT16_MAX. */
> >> uint32_t cutlen; /* length in bytes to cut from the
> >> end. */
> >> ovs_be32 packet_type; /* Packet type as defined in OpenFlow
> >> */
> >> - uint16_t csum_start; /* Position to start checksumming
> >> from. */
> >> - uint16_t csum_offset; /* Offset to place checksum. */
> >> union {
> >> struct pkt_metadata md;
> >> uint64_t data[DP_PACKET_CONTEXT_SIZE / 8];
> >> @@ -1539,34 +1537,12 @@ dp_packet_ol_set_l4_csum_bad(struct dp_packet *p)
> >> *dp_packet_ol_flags_ptr(p) |= DP_PACKET_OL_RX_L4_CKSUM_BAD;
> >> }
> >>
> >> -/* Marks packet 'p' with good integrity if checksum offload locations
> >> - * were provided. In the case of encapsulated packets, these values may
> >> - * be deeper into the packet than OVS might expect. But the packet
> >> - * should still be considered to have good integrity.
> >> - * The 'csum_start' is the offset from the begin of the packet headers.
> >> - * The 'csum_offset' is the offset from start to place the checksum.
> >> - * The csum_start and csum_offset fields are set from the virtio_net_hdr
> >> - * struct that may be provided by a netdev on packet ingress. */
> >> -static inline void
> >> -dp_packet_ol_l4_csum_check_partial(struct dp_packet *p)
> >> -{
> >> - if (p->csum_start && p->csum_offset) {
> >> - dp_packet_ol_set_l4_csum_partial(p);
> >> - }
> >> -}
> >> -
> >> static inline void
> >> dp_packet_reset_packet(struct dp_packet *b, int off)
> >> {
> >> dp_packet_set_size(b, dp_packet_size(b) - off);
> >> dp_packet_set_data(b, ((unsigned char *) dp_packet_data(b) + off));
> >> dp_packet_reset_offsets(b);
> >> -
> >> - if (b->csum_start >= off && b->csum_offset) {
> >> - /* Adjust values for decapsulation. */
> >> - b->csum_start -= off;
> >> - dp_packet_ol_set_l4_csum_partial(b);
> >> - }
> >> }
> >>
> >> static inline uint32_t ALWAYS_INLINE
> >> diff --git a/lib/dpif-netdev-extract-avx512.c
> >> b/lib/dpif-netdev-extract-avx512.c
> >> index 57ca4c71b7..3ae850c2d5 100644
> >> --- a/lib/dpif-netdev-extract-avx512.c
> >> +++ b/lib/dpif-netdev-extract-avx512.c
> >> @@ -776,7 +776,6 @@ mfex_ipv6_set_hwol(struct dp_packet *pkt)
> >> static void
> >> mfex_tcp_set_hwol(struct dp_packet *pkt)
> >> {
> >> - dp_packet_ol_l4_csum_check_partial(pkt);
> >> if (dp_packet_l4_checksum_good(pkt)
> >> || dp_packet_ol_l4_csum_partial(pkt)) {
> >> dp_packet_hwol_set_csum_tcp(pkt);
> >> @@ -786,7 +785,6 @@ mfex_tcp_set_hwol(struct dp_packet *pkt)
> >> static void
> >> mfex_udp_set_hwol(struct dp_packet *pkt)
> >> {
> >> - dp_packet_ol_l4_csum_check_partial(pkt);
> >> if (dp_packet_l4_checksum_good(pkt)
> >> || dp_packet_ol_l4_csum_partial(pkt)) {
> >> dp_packet_hwol_set_csum_udp(pkt);
> >> diff --git a/lib/flow.c b/lib/flow.c
> >> index c2e1e4ad6f..988ae6bfdc 100644
> >> --- a/lib/flow.c
> >> +++ b/lib/flow.c
> >> @@ -1080,7 +1080,6 @@ miniflow_extract(struct dp_packet *packet, struct
> >> miniflow *dst)
> >> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> >> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
> >> }
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> dp_packet_hwol_set_csum_tcp(packet);
> >> @@ -1100,7 +1099,6 @@ miniflow_extract(struct dp_packet *packet, struct
> >> miniflow *dst)
> >> } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> >> dp_packet_update_rss_hash_ipv6_tcp_udp(packet);
> >> }
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> if (tunneling) {
> >> @@ -1118,7 +1116,6 @@ miniflow_extract(struct dp_packet *packet, struct
> >> miniflow *dst)
> >> miniflow_push_be16(mf, tp_dst, sctp->sctp_dst);
> >> miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
> >> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> dp_packet_hwol_set_csum_sctp(packet);
> >> @@ -1309,20 +1306,17 @@ parse_tcp_flags(struct dp_packet *packet,
> >> if (nw_proto == IPPROTO_TCP && size >= TCP_HEADER_LEN) {
> >> const struct tcp_header *tcp = data;
> >>
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> dp_packet_hwol_set_csum_tcp(packet);
> >> }
> >> return TCP_FLAGS(tcp->tcp_ctl);
> >> } else if (nw_proto == IPPROTO_UDP && size >= UDP_HEADER_LEN) {
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> dp_packet_hwol_set_csum_udp(packet);
> >> }
> >> } else if (nw_proto == IPPROTO_SCTP && size >= SCTP_HEADER_LEN) {
> >> - dp_packet_ol_l4_csum_check_partial(packet);
> >> if (dp_packet_l4_checksum_good(packet)
> >> || dp_packet_ol_l4_csum_partial(packet)) {
> >> dp_packet_hwol_set_csum_sctp(packet);
> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> >> index af7438f1ee..c1c0addab7 100644
> >> --- a/lib/netdev-linux.c
> >> +++ b/lib/netdev-linux.c
> >> @@ -89,6 +89,7 @@ COVERAGE_DEFINE(netdev_get_hwaddr);
> >> COVERAGE_DEFINE(netdev_set_hwaddr);
> >> COVERAGE_DEFINE(netdev_get_ethtool);
> >> COVERAGE_DEFINE(netdev_set_ethtool);
> >> +COVERAGE_DEFINE(netdev_linux_unknown_l4_csum);
> >>
> >>
> >> #ifndef IFLA_IF_NETNSID
> >> @@ -7038,6 +7039,70 @@ af_packet_sock(void)
> >> return sock;
> >> }
> >>
> >> +static int
> >> +netdev_linux_parse_packet(struct dp_packet *b, uint16_t *l2_len,
> >> + uint16_t *l3_len, uint16_t *l4proto)
> >> +{
> >> + struct eth_header *eth_hdr;
> >> + ovs_be16 eth_type;
> >> +
> >> + eth_hdr = dp_packet_at(b, 0, ETH_HEADER_LEN);
> >> + if (!eth_hdr) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + *l2_len = ETH_HEADER_LEN;
> >> + eth_type = eth_hdr->eth_type;
> >> + if (eth_type_vlan(eth_type)) {
> >> + struct vlan_header *vlan = dp_packet_at(b, *l2_len,
> >> VLAN_HEADER_LEN);
> >> +
> >> + if (!vlan) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + eth_type = vlan->vlan_next_type;
> >> + *l2_len += VLAN_HEADER_LEN;
> >> + }
> >> +
> >> + if (eth_type == htons(ETH_TYPE_IP)) {
> >> + struct ip_header *ip_hdr = dp_packet_at(b, *l2_len,
> >> IP_HEADER_LEN);
> >> +
> >> + if (!ip_hdr) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + *l3_len = IP_IHL(ip_hdr->ip_ihl_ver) * 4;
> >> + *l4proto = ip_hdr->ip_proto;
> >> + dp_packet_hwol_set_tx_ipv4(b);
> >> + } else if (eth_type == htons(ETH_TYPE_IPV6)) {
> >> + struct ovs_16aligned_ip6_hdr *nh6;
> >> + const void *data;
> >> + uint8_t nw_proto;
> >> + uint8_t nw_frag;
> >> + size_t size;
> >> +
> >> + nh6 = dp_packet_at(b, *l2_len, IPV6_HEADER_LEN);
> >> + if (!nh6) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + nw_proto = nh6->ip6_ctlun.ip6_un1.ip6_un1_nxt;
> >> + data = (const char *) nh6 + sizeof *nh6;
> >> + size = (const char *) dp_packet_tail(b) - (const char *) data;
> >> + if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag,
> >> + NULL, NULL)) {
> >> + return -EINVAL;
> >> + }
> >> + *l3_len = (const char *) data - (const char *) nh6;
> >> + *l4proto = nw_proto;
> >> + dp_packet_hwol_set_tx_ipv6(b);
> >> + } else {
> >> + *l3_len = *l4proto = 0;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> /* Initializes packet 'b' with features enabled in the prepended
> >> * struct virtio_net_hdr. Returns 0 if successful, otherwise a
> >> * positive errno value. */
> >> @@ -7055,16 +7120,44 @@ netdev_linux_parse_vnet_hdr(struct dp_packet *b)
> >> }
> >>
> >> if (vnet->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> >
> > Instead of implementing our own checksumming here, why not just use
> > ethtool to disable all checksum offloading?
>
> Wouldn't that disable most of other offloads as well, like TSO?
> If so, we'll loose a lot of performance.
We can toggle checksum and tso flags independently. I feel like this
approach is slightly more honest than indicating in the ovs-vsctl list
interface that checksums are offloaded, and then manually calculating
them all anyways. And I think doing this in usermode would be far more
impactful than the kernel/hw doing it.
>
> >
> >> - /* The packet has offloaded checksum. However, there is no
> >> - * additional information like the protocol used, so it would
> >> - * require to parse the packet here. The checksum starting point
> >> - * and offset are going to be verified when the packet headers
> >> - * are parsed during miniflow extraction. */
> >> - b->csum_start = (OVS_FORCE uint16_t) vnet->csum_start;
> >> - b->csum_offset = (OVS_FORCE uint16_t) vnet->csum_offset;
> >> - } else {
> >> - b->csum_start = 0;
> >> - b->csum_offset = 0;
> >> + uint16_t csum_offset = (OVS_FORCE uint16_t) vnet->csum_offset;
> >> + uint16_t csum_start = (OVS_FORCE uint16_t) vnet->csum_start;
> >> + uint16_t l4proto;
> >> + uint16_t l2_len;
> >> + uint16_t l3_len;
> >> +
> >> + if (netdev_linux_parse_packet(b, &l2_len, &l3_len, &l4proto)) {
> >> + return EINVAL;
> >> + }
>
> This seems to me like the most contraversial part of the set. It doesn't
> seem wise to have a separate parsing method for this. While we may partially
> rely on kernel to not provide us with some broken packet, I'm not sure we can
> fully trust those packets either. We should be using more robust parsing
> implementation, e.g. call the parse_tcp_flags() function. It will take care
> of multiple vlan headers, IP fragments and potentially broken L3 headers.
> After this call we'll have all the offsets set in the dp_packet itself and
> we'll also have L4 protocol set with the previous patches. So, we should be
> able to use that information below to verify the checksum start and offset.
>
> If we didn't recognize the protocol, then just calculating it below seems
> reasonable.
>
> >> +
> >> + if (csum_start && csum_offset && csum_start == l2_len + l3_len
> >> + && ((csum_offset == offsetof(struct tcp_header, tcp_csum)
> >> + && l4proto == IPPROTO_TCP)
> >> + || (csum_offset == offsetof(struct udp_header, udp_csum)
> >> + && l4proto == IPPROTO_UDP)
> >> + || (csum_offset == offsetof(struct sctp_header, sctp_csum)
> >> + && l4proto == IPPROTO_SCTP))) {
> >> + dp_packet_ol_set_l4_csum_partial(b);
> >> + } else {
> >> + ovs_be16 *csum_l4;
> >> + void *l4;
> >> +
> >> + COVERAGE_INC(netdev_linux_unknown_l4_csum);
> >> +
> >> + csum_l4 = dp_packet_at(b, csum_start + csum_offset,
> >> + sizeof *csum_l4);
> >> + if (!csum_l4) {
> >> + return EINVAL;
> >> + }
> >> +
> >> + l4 = dp_packet_at(b, csum_start, dp_packet_size(b) -
> >> csum_start);
> >> + *csum_l4 = csum(l4, dp_packet_size(b) - csum_start);
> >> +
> >> + if (l4proto == IPPROTO_TCP || l4proto == IPPROTO_UDP
> >> + || l4proto == IPPROTO_SCTP) {
> >> + dp_packet_ol_set_l4_csum_good(b);
> >> + }
> >> + }
> >> }
> >>
> >> int ret = 0;
> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> >> index cbb875bd26..d49597f380 100644
> >> --- a/lib/netdev-native-tnl.c
> >> +++ b/lib/netdev-native-tnl.c
> >> @@ -338,11 +338,7 @@ netdev_tnl_push_udp_header(const struct netdev
> >> *netdev OVS_UNUSED,
> >> } else {
> >> dp_packet_hwol_set_csum_udp(packet);
> >> }
> >> - }
> >> -
> >> - if (packet->csum_start && packet->csum_offset) {
> >> - dp_packet_ol_set_l4_csum_partial(packet);
> >> - } else if (!udp->udp_csum) {
> >> + } else {
> >> dp_packet_ol_set_l4_csum_good(packet);
> >> }
> >>
> >> --
> >> 2.48.1
> >>
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
