From: Jan Klemkow Subject: Re: tcp softlro refactor check To: Alexander Bluhm Cc: tech@openbsd.org Date: Tue, 22 Apr 2025 13:42:36 +0200 On Tue, Apr 22, 2025 at 12:30:24AM GMT, Alexander Bluhm wrote: > Hi, > > I would like to do all checks that affect only a single packet > before looping over the enqueued packets. Fragment and TCP protocol > checks can be removed, ether_extract_headers() already does that. > > After the check is successful, store the payload length in ph_mss. > This is done later anyway. If this mbuf field contains a positive > length, we know that the check has already been done. > > ok? > > bluhm > > Index: netinet/tcp_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v > diff -u -p -r1.437 tcp_input.c > --- netinet/tcp_input.c 21 Apr 2025 09:54:53 -0000 1.437 > +++ netinet/tcp_input.c 21 Apr 2025 20:49:36 -0000 > @@ -4246,6 +4246,42 @@ syn_cache_respond(struct syn_cache *sc, > return (error); > } > > +static int > +tcp_softlro_check(struct mbuf *m, struct ether_extracted *ext) > +{ > + /* Don't merge packets with invalid TCP checksum. */ > + if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK)) > + return 0; > + > + if (ext->ip4) { > + /* Don't merge packets with invalid IP header checksum. */ > + if (!ISSET(m->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK)) > + return 0; > + > + /* Don't merge IPv4 packets with IP options. */ > + if (ext->iphlen != sizeof(struct ip)) > + return 0; > + } > + > + /* Check TCP protocol and header. */ > + if (!ext->tcp) > + return 0; > + > + /* Don't merge empty segments. */ > + if (ext->paylen == 0) > + return 0; > + > + /* Just ACK and PUSH TCP flags are allowed. */ > + if (ISSET(ext->tcp->th_flags, TH_ACK|TH_PUSH) != ext->tcp->th_flags) > + return 0; I know, the internal mechanism of ISSET macro allows this. Please keep it this way: if (ISSET(ext->tcp->th_flags, ~(TH_ACK|TH_PUSH))) return 0; So, it is more readable. The rest the diff looks fine is "OK jan@". Thanks, Jan > + > + /* TCP ACK flag has to be set. */ > + if (!ISSET(ext->tcp->th_flags, TH_ACK)) > + return 0; > + > + return 1; > +} > + > int > tcp_softlro(struct mbuf *mhead, struct mbuf *mtail) > { > @@ -4278,34 +4314,14 @@ tcp_softlro(struct mbuf *mhead, struct m > > /* Check IP header. */ > if (head.ip4 && tail.ip4) { > - /* Don't merge packets with invalid header checksum. */ > - if (!ISSET(mhead->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK) || > - !ISSET(mtail->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK)) > - return 0; > - > /* Check IPv4 addresses. */ > if (head.ip4->ip_src.s_addr != tail.ip4->ip_src.s_addr || > head.ip4->ip_dst.s_addr != tail.ip4->ip_dst.s_addr) > return 0; > > - /* Don't merge IPv4 fragments. */ > - if (ISSET(head.ip4->ip_off, htons(IP_OFFMASK | IP_MF)) || > - ISSET(tail.ip4->ip_off, htons(IP_OFFMASK | IP_MF))) > - return 0; > - > /* Check max. IPv4 length. */ > if (head.iplen + tail.iplen > IP_MAXPACKET) > return 0; > - > - /* Don't merge IPv4 packets with option headers. */ > - if (head.iphlen != sizeof(struct ip) || > - tail.iphlen != sizeof(struct ip)) > - return 0; > - > - /* Don't non-TCP packets. */ > - if (head.ip4->ip_p != IPPROTO_TCP || > - tail.ip4->ip_p != IPPROTO_TCP) > - return 0; > } else if (head.ip6 && tail.ip6) { > /* Check IPv6 addresses. */ > if (!IN6_ARE_ADDR_EQUAL(&head.ip6->ip6_src, &tail.ip6->ip6_src) || > @@ -4316,19 +4332,10 @@ tcp_softlro(struct mbuf *mhead, struct m > if ((head.iplen - head.iphlen) + > (tail.iplen - tail.iphlen) > IPV6_MAXPACKET) > return 0; > - > - /* Don't merge IPv6 packets with option headers nor non-TCP. */ > - if (head.ip6->ip6_nxt != IPPROTO_TCP || > - tail.ip6->ip6_nxt != IPPROTO_TCP) > - return 0; > } else { > return 0; > } > > - /* Check TCP header. */ > - if (!head.tcp || !tail.tcp) > - return 0; > - > /* Check TCP ports. */ > if (head.tcp->th_sport != tail.tcp->th_sport || > head.tcp->th_dport != tail.tcp->th_dport) > @@ -4342,16 +4349,6 @@ tcp_softlro(struct mbuf *mhead, struct m > if (ntohl(head.tcp->th_seq) + head.paylen != ntohl(tail.tcp->th_seq)) > return 0; > > - /* Just ACK and PUSH TCP flags are allowed. */ > - if (ISSET(head.tcp->th_flags, ~(TH_ACK|TH_PUSH)) || > - ISSET(tail.tcp->th_flags, ~(TH_ACK|TH_PUSH))) > - return 0; > - > - /* TCP ACK flag has to be set. */ > - if (!ISSET(head.tcp->th_flags, TH_ACK) || > - !ISSET(tail.tcp->th_flags, TH_ACK)) > - return 0; > - > /* Ignore segments with different TCP options. */ > if (head.tcphlen - sizeof(struct tcphdr) != > tail.tcphlen - sizeof(struct tcphdr)) > @@ -4454,19 +4451,23 @@ tcp_softlro(struct mbuf *mhead, struct m > void > tcp_softlro_glue(struct mbuf_list *ml, struct mbuf *mtail, struct ifnet *ifp) > { > + struct ether_extracted tail; > struct mbuf *mhead; > > if (!ISSET(ifp->if_xflags, IFXF_LRO)) > goto out; > > - /* Don't merge packets with invalid header checksum. */ > - if (!ISSET(mtail->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK)) > - goto out; > + ether_extract_headers(mtail, &tail); > + if (!tcp_softlro_check(mtail, &tail)) { > + mtail->m_pkthdr.ph_mss = 0; > + goto out; > + } > + mtail->m_pkthdr.ph_mss = tail.paylen; > > for (mhead = ml->ml_head; mhead != NULL; mhead = mhead->m_nextpkt) { > - /* Don't merge packets with invalid header checksum. */ > - if (!ISSET(mhead->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK)) > - continue; > + /* This packet has been checked and was not mergable before. */ > + if (mhead->m_pkthdr.ph_mss == 0) > + continue; > > /* Use RSS hash to skip packets of different connections. */ > if (ISSET(mhead->m_pkthdr.csum_flags, M_FLOWID) && >