Download raw body.
tcp softlro refactor check
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) &&
>
tcp softlro refactor check