Index | Thread | Search

From:
Jan Klemkow <jan@openbsd.org>
Subject:
Re: softlro refactor extract header
To:
Alexander Bluhm <bluhm@openbsd.org>
Cc:
tech@openbsd.org
Date:
Wed, 23 Apr 2025 19:28:54 +0200

Download raw body.

Thread
On Wed, Apr 23, 2025 at 07:11:35PM +0200, Alexander Bluhm wrote:
> The header of the tail mbuf has already been extraced.  Do all
> header extraction in tcp_softlro_glue() and pass them down to
> tcp_softlro().
> 
> Also do port check before address check as IPv6 address comparison
> is expensive.  The empty segment check has already been done in
> tcp_softlro_check(), remove it from tcp_softlro().
> 
> ok?

ok jan

> Index: netinet/tcp_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
> diff -u -p -r1.439 tcp_input.c
> --- netinet/tcp_input.c	22 Apr 2025 22:34:27 -0000	1.439
> +++ netinet/tcp_input.c	23 Apr 2025 17:02:26 -0000
> @@ -4283,10 +4283,9 @@ tcp_softlro_check(struct mbuf *m, struct
>  }
>  
>  int
> -tcp_softlro(struct mbuf *mhead, struct mbuf *mtail)
> +tcp_softlro(struct mbuf *mhead, struct ether_extracted *head,
> +    struct mbuf *mtail, struct ether_extracted *tail)
>  {
> -	struct ether_extracted	 head;
> -	struct ether_extracted	 tail;
>  	struct mbuf		*m;
>  	unsigned int		 hdrlen;
>  	unsigned int		 cnt = 0;
> @@ -4295,70 +4294,66 @@ tcp_softlro(struct mbuf *mhead, struct m
>  	 * Check if head and tail are mergeable
>  	 */
>  
> -	ether_extract_headers(mhead, &head);
> -	ether_extract_headers(mtail, &tail);
> -
>  	/* Don't merge packets inside and outside of VLANs */
> -	if (head.evh && tail.evh) {
> +	if (head->evh && tail->evh) {
>  		/* Don't merge packets of different VLANs */
> -		if (EVL_VLANOFTAG(head.evh->evl_tag) !=
> -		    EVL_VLANOFTAG(tail.evh->evl_tag))
> +		if (EVL_VLANOFTAG(head->evh->evl_tag) !=
> +		    EVL_VLANOFTAG(tail->evh->evl_tag))
>  			return 0;
>  
>  		/* Don't merge packets of different priorities */
> -		if (EVL_PRIOFTAG(head.evh->evl_tag) !=
> -		    EVL_PRIOFTAG(tail.evh->evl_tag))
> +		if (EVL_PRIOFTAG(head->evh->evl_tag) !=
> +		    EVL_PRIOFTAG(tail->evh->evl_tag))
>  			return 0;
> -	} else if (head.evh || tail.evh)
> +	} else if (head->evh || tail->evh)
> +		return 0;
> +
> +	/* Check TCP ports. */
> +	if (head->tcp->th_sport != tail->tcp->th_sport ||
> +	    head->tcp->th_dport != tail->tcp->th_dport)
>  		return 0;
>  
>  	/* Check IP header. */
> -	if (head.ip4 && tail.ip4) {
> +	if (head->ip4 && tail->ip4) {
>  		/* 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)
> +		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;
>  
>  		/* Check max. IPv4 length. */
> -		if (head.iplen + tail.iplen > IP_MAXPACKET)
> +		if (head->iplen + tail->iplen > IP_MAXPACKET)
>  			return 0;
> -	} else if (head.ip6 && tail.ip6) {
> +	} else if (head->ip6 && tail->ip6) {
>  		/* Check IPv6 addresses. */
> -		if (!IN6_ARE_ADDR_EQUAL(&head.ip6->ip6_src, &tail.ip6->ip6_src) ||
> -		    !IN6_ARE_ADDR_EQUAL(&head.ip6->ip6_dst, &tail.ip6->ip6_dst))
> +		if (!IN6_ARE_ADDR_EQUAL(&head->ip6->ip6_src,
> +		    &tail->ip6->ip6_src) ||
> +		    !IN6_ARE_ADDR_EQUAL(&head->ip6->ip6_dst,
> +		    &tail->ip6->ip6_dst))
>  			return 0;
>  
>  		/* Check max. IPv6 length. */
> -		if ((head.iplen - head.iphlen) +
> -		    (tail.iplen - tail.iphlen) > IPV6_MAXPACKET)
> +		if ((head->iplen - head->iphlen) +
> +		    (tail->iplen - tail->iphlen) > IPV6_MAXPACKET)
>  			return 0;
>  	} else {
> +		/* Address family does not match. */
>  		return 0;
>  	}
>  
> -	/* Check TCP ports. */
> -	if (head.tcp->th_sport != tail.tcp->th_sport ||
> -	    head.tcp->th_dport != tail.tcp->th_dport)
> -		return 0;
> -
> -	/* Don't merge empty segments. */
> -	if (head.paylen == 0 || tail.paylen == 0)
> -		return 0;
> -
>  	/* Check for continues segments. */
> -	if (ntohl(head.tcp->th_seq) + head.paylen != ntohl(tail.tcp->th_seq))
> +	if (ntohl(head->tcp->th_seq) + head->paylen != ntohl(tail->tcp->th_seq))
>  		return 0;
>  
>  	/* Ignore segments with different TCP options. */
> -	if (head.tcphlen - sizeof(struct tcphdr) !=
> -	    tail.tcphlen - sizeof(struct tcphdr))
> +	if (head->tcphlen - sizeof(struct tcphdr) !=
> +	    tail->tcphlen - sizeof(struct tcphdr))
>  		return 0;
>  
>  	/* Check for TCP options */
> -	if (head.tcphlen > sizeof(struct tcphdr)) {
> -		char *hopt = (char *)(head.tcp) + sizeof(struct tcphdr);
> -		char *topt = (char *)(tail.tcp) + sizeof(struct tcphdr);
> -		int optsize = head.tcphlen - sizeof(struct tcphdr);
> +	if (head->tcphlen > sizeof(struct tcphdr)) {
> +		char *hopt = (char *)(head->tcp) + sizeof(struct tcphdr);
> +		char *topt = (char *)(tail->tcp) + sizeof(struct tcphdr);
> +		int optsize = head->tcphlen - sizeof(struct tcphdr);
>  		int optlen;
>  
>  		for (; optsize > 0; optsize -= optlen) {
> @@ -4395,27 +4390,27 @@ tcp_softlro(struct mbuf *mhead, struct m
>  	 */
>  
>  	/* Adjust IP header. */
> -	if (head.ip4) {
> -		head.ip4->ip_len = htons(head.iplen + tail.paylen);
> -	} else if (head.ip6) {
> -		head.ip6->ip6_plen =
> -		    htons(head.iplen - head.iphlen + tail.paylen);
> +	if (head->ip4) {
> +		head->ip4->ip_len = htons(head->iplen + tail->paylen);
> +	} else if (head->ip6) {
> +		head->ip6->ip6_plen =
> +		    htons(head->iplen - head->iphlen + tail->paylen);
>  	}
>  
>  	/* Combine TCP flags from head and tail. */
> -	if (ISSET(tail.tcp->th_flags, TH_PUSH))
> -		SET(head.tcp->th_flags, TH_PUSH);
> +	if (ISSET(tail->tcp->th_flags, TH_PUSH))
> +		SET(head->tcp->th_flags, TH_PUSH);
>  
>  	/* Adjust TCP header. */
> -	head.tcp->th_win = tail.tcp->th_win;
> -	head.tcp->th_ack = tail.tcp->th_ack;
> +	head->tcp->th_win = tail->tcp->th_win;
> +	head->tcp->th_ack = tail->tcp->th_ack;
>  
>  	/* Calculate header length of tail packet. */
> -	hdrlen = sizeof(*tail.eh);
> -	if (tail.evh)
> -		hdrlen = sizeof(*tail.evh);
> -	hdrlen += tail.iphlen;
> -	hdrlen += tail.tcphlen;
> +	hdrlen = sizeof(*tail->eh);
> +	if (tail->evh)
> +		hdrlen = sizeof(*tail->evh);
> +	hdrlen += tail->iphlen;
> +	hdrlen += tail->tcphlen;
>  
>  	/* Skip protocol headers in tail. */
>  	m_adj(mtail, hdrlen);
> @@ -4425,24 +4420,24 @@ tcp_softlro(struct mbuf *mhead, struct m
>  	for (m = mhead; m->m_next;)
>  		m = m->m_next;
>  	m->m_next = mtail;
> -	mhead->m_pkthdr.len += tail.paylen;
> +	mhead->m_pkthdr.len += tail->paylen;
>  
>  	/* Flag mbuf as TSO packet with MSS. */
>  	if (!ISSET(mhead->m_pkthdr.csum_flags, M_TCP_TSO)) {
>  		/* Set CSUM_OUT flags in case of forwarding. */
>  		SET(mhead->m_pkthdr.csum_flags, M_TCP_CSUM_OUT);
> -		head.tcp->th_sum = 0;
> -		if (head.ip4) {
> +		head->tcp->th_sum = 0;
> +		if (head->ip4) {
>  			SET(mhead->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT);
> -			head.ip4->ip_sum = 0;
> +			head->ip4->ip_sum = 0;
>  		}
>  
>  		SET(mhead->m_pkthdr.csum_flags, M_TCP_TSO);
> -		mhead->m_pkthdr.ph_mss = head.paylen;
> +		mhead->m_pkthdr.ph_mss = head->paylen;
>  		tcpstat_inc(tcps_inswlro);
>  		tcpstat_inc(tcps_inpktlro);	/* count head */
>  	}
> -	mhead->m_pkthdr.ph_mss = MAX(mhead->m_pkthdr.ph_mss, tail.paylen);
> +	mhead->m_pkthdr.ph_mss = MAX(mhead->m_pkthdr.ph_mss, tail->paylen);
>  	tcpstat_inc(tcps_inpktlro);	/* count tail */
>  
>  	return 1;
> @@ -4451,7 +4446,7 @@ 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 ether_extracted head, tail;
>  	struct mbuf *mhead;
>  
>  	if (!ISSET(ifp->if_xflags, IFXF_LRO))
> @@ -4492,7 +4487,8 @@ tcp_softlro_glue(struct mbuf_list *ml, s
>  				continue;
>  		}
>  
> -		if (tcp_softlro(mhead, mtail))
> +		ether_extract_headers(mhead, &head);
> +		if (tcp_softlro(mhead, &head, mtail, &tail))
>  			return;
>  	}
>   out:
>