Re: [PATCH v5 4/5] tcp: Update TCP checksum using an iovec array

On Tue, 1 Oct 2024 09:29:28 +0200
Stefano Brivio <sbrivio(a)redhat.com> wrote:

...
  On Fri, 27 Sep 2024 15:53:48 +0200
 Laurent Vivier <lvivier(a)redhat.com> wrote:
 
  [...]

 +void tcp_update_check_tcp4(const struct iphdr *iph,
 +			   const struct iovec *iov, int iov_cnt,
 +			   size_t l4offset)
  {
  	uint16_t l4len = ntohs(iph->tot_len) - sizeof(struct iphdr);
  	struct in_addr saddr = { .s_addr = iph->saddr };
  	struct in_addr daddr = { .s_addr = iph->daddr };
 -	uint32_t sum = proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, daddr);
 +	size_t check_ofs;
 +	__sum16 *check;
 +	int check_idx;
 +	uint32_t sum;
 +	uintptr_t ptr;
 +
 +	sum = proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, daddr);
 +
 +	check_idx = iov_skip_bytes(iov, iov_cnt,
 +				   l4offset + offsetof(struct tcphdr, check),
 +				   &check_ofs);
 +
 +	if (check_idx >= iov_cnt) {
 +		err("TCP4 buffer is too small, iov size %zd, check offset %zd",
 +		    iov_size(iov, iov_cnt),
 +		    l4offset + offsetof(struct tcphdr, check));
 +		return;
 +	}
  
 -	bp->th.check = 0;
 -	bp->th.check = csum(bp, l4len, sum);
 +	if (check_ofs + sizeof(*check) > iov[check_idx].iov_len) {
 +		err("TCP4 checksum field memory is not contiguous "
 +		    "check_ofs %zd check_idx %d iov_len %zd",
 +		    check_ofs, check_idx, iov[check_idx].iov_len);
 +		return;
 +	}
 +
 +	ptr = (uintptr_t)((char *)iov[check_idx].iov_base + check_ofs);
 +	if (ptr & (__alignof__(*check) - 1)) {
 +		err("TCP4 checksum field is not correctly aligned in memory");
 +		return;
 +	}
 +
 +	check = (__sum16 *)ptr;   
 
 clang-tidy (14.0.6) says:
 
 /home/sbrivio/passt/tcp.c:803:10: error: integer to pointer cast pessimizes optimization
opportunities [performance-no-int-to-ptr,-warnings-as-errors]
         check = (__sum16 *)ptr;
                 ^
 /home/sbrivio/passt/tcp.c:854:10: error: integer to pointer cast pessimizes optimization
opportunities [performance-no-int-to-ptr,-warnings-as-errors]
         check = (__sum16 *)ptr;
                 ^
 
 ...I'm still trying to figure out what that actually means. If it's
 trivial I can fix that up on merge. 
Something like this?

--
diff --git a/tcp.c b/tcp.c
index ffd82e1..2b8edda 100644
--- a/tcp.c
+++ b/tcp.c
@@ -772,7 +772,7 @@ void tcp_update_check_tcp4(const struct iphdr *iph,
 	__sum16 *check;
 	int check_idx;
 	uint32_t sum;
-	uintptr_t ptr;
+	char *ptr;
 
 	sum = proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, daddr);
 
@@ -794,8 +794,8 @@ void tcp_update_check_tcp4(const struct iphdr *iph,
 		return;
 	}
 
-	ptr = (uintptr_t)((char *)iov[check_idx].iov_base + check_ofs);
-	if (ptr & (__alignof__(*check) - 1)) {
+	ptr = (char *)iov[check_idx].iov_base + check_ofs;
+	if ((uintptr_t)ptr & (__alignof__(*check) - 1)) {
 		err("TCP4 checksum field is not correctly aligned in memory");
 		return;
 	}
@@ -822,7 +822,7 @@ void tcp_update_check_tcp6(const struct ipv6hdr *ip6h,
 	__sum16 *check;
 	int check_idx;
 	uint32_t sum;
-	uintptr_t ptr;
+	char *ptr;
 
 	sum = proto_ipv6_header_psum(l4len, IPPROTO_TCP, &ip6h->saddr,
 				     &ip6h->daddr);
@@ -845,8 +845,8 @@ void tcp_update_check_tcp6(const struct ipv6hdr *ip6h,
 		return;
 	}
 
-	ptr = (uintptr_t)((char *)iov[check_idx].iov_base + check_ofs);
-	if (ptr & (__alignof__(*check) - 1)) {
+	ptr = (char *)iov[check_idx].iov_base + check_ofs;
+	if ((uintptr_t)ptr & (__alignof__(*check) - 1)) {
 		err("TCP6 checksum field is not correctly aligned in memory");
 		return;
 	}

-- 

it doesn't really improve on the "optimization opportunities" reported
by clang-tidy but it silences the warning without much fuss. I can
apply this on top if it looks okay to you.

-- 
Stefano