From: jan@openbsd.org Subject: Re: vio(4): tso To: Alexander Bluhm Cc: Brian Conway , tech@openbsd.org Date: Tue, 9 Apr 2024 14:51:27 +0200 On Mon, Apr 08, 2024 at 08:37:11PM +0200, Alexander Bluhm wrote: > On Fri, Apr 05, 2024 at 08:51:48PM +0200, jan@openbsd.org wrote: > > On Sun, Mar 03, 2024 at 05:13:26PM +0100, jan@openbsd.org wrote: > > > On Thu, Feb 29, 2024 at 07:52:23AM -0600, Brian Conway wrote: > > > > Original thread, sorry for breaking threading: https://marc.info/?l=openbsd-tech&m=170523759201797 > > > > > > > > Greetings. Are you still looking for testing on this? I went to give > > > > it a spin today, but it no longer applies cleanly due to other recent > > > > vio(4) work. > > > > > > I will update and resend the diff after the release of 7.5. So, there > > > it more time for testing. Before the 7.6 release. > > > > Here is an updated version of the vio(4) TSO diff. It takes some > > advantage of the ether_extract_headers() improvements. vlan(4) in > > combination with vio(4) will work, but does not use TSO for now. Thus, > > no performance increase with vlan traffic. > > > > I tested it with IPv4 and IPv6 on a Linux KVM host. > > With tcpbench sending for OpenBSD to Linux I get an increase of > factor 7. OpenBSD and Linux guest run in the same KVM host. > > > Tests are welcome. > > No regression seen. > > > ok? > > some remarks: > > > +static inline uint16_t > > +vio_tso_cksum(uint32_t cksum, uint16_t paylen) > > +{ > > + /* Add payload length */ > > + cksum += paylen; > > + > > + /* Fold back to 16 bit */ > > + cksum += cksum >> 16; > > + > > + return (uint16_t)(cksum); > > +} > > This is a generic checksum update function that is not TSO specific. > > Can we call it vio_cksum_update()? The htons belongs in the caller, > not calle. And the parameter is not a specific paylen, but a generic > update value. done > > +void > > +vio_tx_offload(struct virtio_net_hdr *hdr, struct mbuf *m) > > +{ > > + struct ether_extracted ext; > > + > > + /* > > + * Checksum Offload > > + */ > > + > > + if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) && > > + !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) > > + return; > > + > > + ether_extract_headers(m, &ext); > > + hdr->csum_start = sizeof(*ext.eh); > > +#if NVLAN > 0 > > + if (ext.evh) > > + hdr->csum_start = sizeof(*ext.evh); > > +#endif > > + if (ext.ip4 || ext.ip6) > > + hdr->csum_start += ext.iphlen; > > + > > + if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) > > + hdr->csum_offset = offsetof(struct tcphdr, th_sum); > > + else > > + hdr->csum_offset = offsetof(struct udphdr, uh_sum); > > Use ISSET consistently. done > Can we be a bit more paranois here? done > if (ext.tcp && ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) > hdr->csum_offset = offsetof(struct tcphdr, th_sum); > else if (ext.udp && ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) > hdr->csum_offset = offsetof(struct udphdr, uh_sum); > else > return; > > And move the consistency check before modifying any hdr fields. done > > + hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM; > > + > > + /* > > + * TCP Segmentation Offload > > + */ > > + > > + if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) > > + return; > > + > > + hdr->hdr_len = hdr->csum_start + ext.tcphlen; > > + hdr->gso_size = m->m_pkthdr.ph_mss; > > + > > + if (ext.ip4) > > + hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > +#ifdef INET6 > > + else if (ext.ip6) > > + hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > +#endif > > + else > > + tcpstat_inc(tcps_outbadtso); > > Here a return is missing. Do no try TSO for non IP packets. Put > the check, error counter and return before touching any hdr fields. > If ext.tcp == NULL, also report an error. done > > + /* VirtIO-Net need pseudo header cksum with IP-payload length for TSO */ > > + ext.tcp->th_sum = vio_tso_cksum(ext.tcp->th_sum, > > + htons(ext.iplen - ext.iphlen)); > > + > > + tcpstat_add(tcps_outpkttso, > > + (ext.paylen + m->m_pkthdr.ph_mss - 1) / m->m_pkthdr.ph_mss); > > +} > > + ok? Thanks, Jan Index: dev/pv/if_vio.c =================================================================== RCS file: /cvs/src/sys/dev/pv/if_vio.c,v diff -u -p -r1.31 if_vio.c --- dev/pv/if_vio.c 14 Feb 2024 22:41:48 -0000 1.31 +++ dev/pv/if_vio.c 9 Apr 2024 12:40:11 -0000 @@ -43,12 +43,15 @@ #include #include +#include #include #include #include #include #include +#include +#include #include #if NBPFILTER > 0 @@ -537,6 +540,9 @@ vio_attach(struct device *parent, struct VIRTIO_NET_F_MRG_RXBUF | VIRTIO_NET_F_CSUM | VIRTIO_F_RING_EVENT_IDX | VIRTIO_NET_F_GUEST_CSUM; + vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO4; + vsc->sc_driver_features |= VIRTIO_NET_F_HOST_TSO6; + virtio_negotiate_features(vsc, virtio_net_feature_names); if (virtio_has_feature(vsc, VIRTIO_NET_F_MAC)) { vio_get_lladdr(&sc->sc_ac, vsc); @@ -553,9 +559,9 @@ vio_attach(struct device *parent, struct sc->sc_hdr_size = offsetof(struct virtio_net_hdr, num_buffers); } if (virtio_has_feature(vsc, VIRTIO_NET_F_MRG_RXBUF)) - ifp->if_hardmtu = 16000; /* arbitrary limit */ + ifp->if_hardmtu = MAXMCLBYTES; else - ifp->if_hardmtu = MCLBYTES - sc->sc_hdr_size - ETHER_HDR_LEN; + ifp->if_hardmtu = MAXMCLBYTES - sc->sc_hdr_size - ETHER_HDR_LEN; if (virtio_alloc_vq(vsc, &sc->sc_vq[VQRX], 0, MCLBYTES, 2, "rx") != 0) goto err; @@ -595,6 +601,10 @@ vio_attach(struct device *parent, struct if (virtio_has_feature(vsc, VIRTIO_NET_F_CSUM)) ifp->if_capabilities |= IFCAP_CSUM_TCPv4|IFCAP_CSUM_UDPv4| IFCAP_CSUM_TCPv6|IFCAP_CSUM_UDPv6; + if (virtio_has_feature(vsc, VIRTIO_NET_F_HOST_TSO4)) + ifp->if_capabilities |= IFCAP_TSOv4; + if (virtio_has_feature(vsc, VIRTIO_NET_F_HOST_TSO6)) + ifp->if_capabilities |= IFCAP_TSOv6; ifq_init_maxlen(&ifp->if_snd, vsc->sc_vqs[1].vq_num - 1); ifmedia_init(&sc->sc_media, 0, vio_media_change, vio_media_status); ifmedia_add(&sc->sc_media, IFM_ETHER | IFM_AUTO, 0, NULL); @@ -714,6 +724,89 @@ vio_stop(struct ifnet *ifp, int disable) } } +static inline uint16_t +vio_cksum_update(uint32_t cksum, uint16_t paylen) +{ + /* Add payload length */ + cksum += paylen; + + /* Fold back to 16 bit */ + cksum += cksum >> 16; + + return (uint16_t)(cksum); +} + +void +vio_tx_offload(struct virtio_net_hdr *hdr, struct mbuf *m) +{ + struct ether_extracted ext; + + /* + * Checksum Offload + */ + + if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) && + !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) + return; + + ether_extract_headers(m, &ext); + + /* Consistency Checks */ + if ((!ext.ip4 && !ext.ip6) || (!ext.tcp && !ext.udp)) + return; + + if (ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) && + ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) + return; + + if ((ext.tcp && !ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) || + (ext.udp && !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT))) + return; + + if (ext.udp && ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) { + tcpstat_inc(tcps_outbadtso); + return; + } + + hdr->csum_start = sizeof(*ext.eh); +#if NVLAN > 0 + if (ext.evh) + hdr->csum_start = sizeof(*ext.evh); +#endif + hdr->csum_start += ext.iphlen; + + if (ext.tcp) + hdr->csum_offset = offsetof(struct tcphdr, th_sum); + else if (ext.udp) + hdr->csum_offset = offsetof(struct udphdr, uh_sum); + + hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM; + + /* + * TCP Segmentation Offload + */ + + if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) + return; + + hdr->hdr_len = hdr->csum_start + ext.tcphlen; + hdr->gso_size = m->m_pkthdr.ph_mss; + + if (ext.ip4) + hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; +#ifdef INET6 + else if (ext.ip6) + hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; +#endif + + /* VirtIO-Net need pseudo header cksum with IP-payload length for TSO */ + ext.tcp->th_sum = vio_cksum_update(ext.tcp->th_sum, + htons(ext.iplen - ext.iphlen)); + + tcpstat_add(tcps_outpkttso, + (ext.paylen + m->m_pkthdr.ph_mss - 1) / m->m_pkthdr.ph_mss); +} + void vio_start(struct ifnet *ifp) { @@ -750,24 +843,7 @@ again: hdr = &sc->sc_tx_hdrs[slot]; memset(hdr, 0, sc->sc_hdr_size); - if (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT|M_UDP_CSUM_OUT)) { - struct ether_extracted ext; - - ether_extract_headers(m, &ext); - hdr->csum_start = sizeof(*ext.eh); -#if NVLAN > 0 - if (ext.evh) - hdr->csum_start = sizeof(*ext.evh); -#endif - if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) - hdr->csum_offset = offsetof(struct tcphdr, th_sum); - else - hdr->csum_offset = offsetof(struct udphdr, uh_sum); - - if (ext.ip4 || ext.ip6) - hdr->csum_start += ext.iphlen; - hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM; - } + vio_tx_offload(hdr, m); r = vio_encap(sc, slot, m); if (r != 0) {