From: Jan Klemkow <jan@openbsd.org>
Subject: Re: TCP softlro split compare and concat function
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 9 May 2025 18:42:39 +0200

On Thu, May 08, 2025 at 11:50:19PM +0200, Alexander Bluhm wrote:
> I would like to split the function that compares two mbuf chains
> if their TCP segments fit together from the actual concatenation.
> This clarifies where the point of no return is.
> 
> The loops that limit the length of the mbuf chains can be made a
> bit more efficient.
> 
> Also do some minor style nits.
> 
> ok?

ok jan

> Index: netinet/tcp_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/tcp_input.c,v
> diff -u -p -r1.445 tcp_input.c
> --- netinet/tcp_input.c	8 May 2025 20:22:56 -0000	1.445
> +++ netinet/tcp_input.c	8 May 2025 20:55:12 -0000
> @@ -4323,7 +4323,7 @@ tcp_softlro_check(struct mbuf *m, struct
>  	if (!ext->tcp)
>  		return 0;
>  
> -	/* Don't merge empty segments. */
> +	/* Don't merge empty TCP segments. */
>  	if (ext->paylen == 0)
>  		return 0;
>  
> @@ -4351,18 +4351,9 @@ tcp_softlro_check(struct mbuf *m, struct
>  	return 1;
>  }
>  
> -int
> -tcp_softlro(struct mbuf *mhead, struct ether_extracted *head,
> -    struct mbuf *mtail, struct ether_extracted *tail)
> +static int
> +tcp_softlro_compare(struct ether_extracted *head, struct ether_extracted *tail)
>  {
> -	struct mbuf		*m;
> -	unsigned int		 hdrlen;
> -	unsigned int		 cnt = 0;
> -
> -	/*
> -	 * Check if head and tail are mergeable
> -	 */
> -
>  	/* Don't merge packets inside and outside of VLANs */
>  	if (head->evh && tail->evh) {
>  		/* Don't merge packets of different VLANs */
> @@ -4409,7 +4400,7 @@ tcp_softlro(struct mbuf *mhead, struct e
>  		return 0;
>  	}
>  
> -	/* Check for continues segments. */
> +	/* Check for contiguous segments. */
>  	if (ntohl(head->tcp->th_seq) + head->paylen != ntohl(tail->tcp->th_seq))
>  		return 0;
>  
> @@ -4428,19 +4419,17 @@ tcp_softlro(struct mbuf *mhead, struct e
>  			return 0;
>  	}
>  
> -	/* Limit mbuf chain len to avoid m_defrag calls on forwarding. */
> -	for (m = mhead; m != NULL; m = m->m_next)
> -		if (cnt++ >= 8)
> -			return 0;
> -	for (m = mtail; m != NULL; m = m->m_next)
> -		if (cnt++ >= 8)
> -			return 0;
> +	return 1;
> +}
>  
> -	/*
> -	 * Prepare concatenation of head and tail.
> -	 */
> +static void
> +tcp_softlro_concat(struct mbuf *mhead, struct ether_extracted *head,
> +    struct mbuf *mtail, struct ether_extracted *tail)
> +{
> +	struct mbuf *m;
> +	unsigned int hdrlen;
>  
> -	/* Adjust IP header. */
> +	/* Adjust IP header length. */
>  	if (head->ip4) {
>  		head->ip4->ip_len = htons(head->iplen + tail->paylen);
>  	} else if (head->ip6) {
> @@ -4477,8 +4466,8 @@ tcp_softlro(struct mbuf *mhead, struct e
>  	CLR(mtail->m_flags, M_PKTHDR);
>  
>  	/* Concatenate */
> -	for (m = mhead; m->m_next;)
> -		m = m->m_next;
> +	for (m = mhead; m->m_next != NULL; m = m->m_next)
> +		;
>  	m->m_next = mtail;
>  	mhead->m_pkthdr.len += tail->paylen;
>  
> @@ -4499,18 +4488,19 @@ tcp_softlro(struct mbuf *mhead, struct e
>  	}
>  	mhead->m_pkthdr.ph_mss = MAX(mhead->m_pkthdr.ph_mss, tail->paylen);
>  	tcpstat_inc(tcps_inpktlro);	/* count tail */
> -
> -	return 1;
>  }
>  
>  void
>  tcp_softlro_glue(struct mbuf_list *ml, struct mbuf *mtail, struct ifnet *ifp)
>  {
>  	struct ether_extracted head, tail;
> -	struct mbuf *mhead;
> +	struct mbuf *m, *mhead;
> +	unsigned int headcnt, tailcnt;
>  
>  	if (!ISSET(ifp->if_xflags, IFXF_LRO))
> -		goto out;
> +		goto dontmerge;
> +
> +	mtail->m_pkthdr.ph_mss = 0;
>  
>  	ether_extract_headers(mtail, &tail);
>  
> @@ -4525,10 +4515,15 @@ tcp_softlro_glue(struct mbuf_list *ml, s
>  		}
>  	}
>  
> -	if (!tcp_softlro_check(mtail, &tail)) {
> -		mtail->m_pkthdr.ph_mss = 0;
> -		goto out;
> +	if (!tcp_softlro_check(mtail, &tail))
> +		goto dontmerge;
> +
> +	tailcnt = 0;
> +	for (m = mtail; m != NULL; m = m->m_next) {
> +		if (tailcnt++ >= 8)
> +			goto dontmerge;
>  	}
> +
>  	mtail->m_pkthdr.ph_mss = tail.paylen;
>  
>  	for (mhead = ml->ml_head; mhead != NULL; mhead = mhead->m_nextpkt) {
> @@ -4560,9 +4555,19 @@ tcp_softlro_glue(struct mbuf_list *ml, s
>  		}
>  
>  		ether_extract_headers(mhead, &head);
> -		if (tcp_softlro(mhead, &head, mtail, &tail))
> -			return;
> +		if (!tcp_softlro_compare(&head, &tail))
> +			continue;
> +
> +		/* Limit mbuf chain to avoid m_defrag calls when forwarding. */
> +		headcnt = tailcnt;
> +		for (m = mhead; m != NULL; m = m->m_next) {
> +			if (headcnt++ >= 8)
> +				goto dontmerge;
> +		}
> +
> +		tcp_softlro_concat(mhead, &head, mtail, &tail);
> +		return;
>  	}
> - out:
> + dontmerge:
>  	ml_enqueue(ml, mtail);
>  }
>