From: Stefano Brivio <sbrivio@redhat.com>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v5 4/5] tcp: Update TCP checksum using an iovec array
Date: Tue, 1 Oct 2024 20:22:16 +0200 [thread overview]
Message-ID: <20241001202216.5c3ee274@elisabeth> (raw)
In-Reply-To: <20241001092928.33755f2b@elisabeth>
On Tue, 1 Oct 2024 09:29:28 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:
> On Fri, 27 Sep 2024 15:53:48 +0200
> Laurent Vivier <lvivier@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;
}
--
@@ -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
@@ -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
@@ -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
next prev parent reply other threads:[~2024-10-01 18:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-27 13:53 [PATCH v5 0/5] tcp: use csum_iov() in tcp_update_check_tcp[4|6]() Laurent Vivier
2024-09-27 13:53 ` [PATCH v5 1/5] tcp: Use tcp_payload_t rather than tcphdr Laurent Vivier
2024-09-27 13:53 ` [PATCH v5 2/5] pcap: Add an offset argument in pcap_iov() Laurent Vivier
2024-09-27 13:53 ` [PATCH v5 3/5] checksum: Add an offset argument in csum_iov() Laurent Vivier
2024-09-30 2:51 ` David Gibson
2024-09-27 13:53 ` [PATCH v5 4/5] tcp: Update TCP checksum using an iovec array Laurent Vivier
2024-09-30 2:56 ` David Gibson
2024-10-01 7:29 ` Stefano Brivio
2024-10-01 18:22 ` Stefano Brivio [this message]
2024-10-02 7:39 ` Laurent Vivier
2024-10-02 9:00 ` Stefano Brivio
2024-10-03 12:58 ` Laurent Vivier
2024-09-27 13:53 ` [PATCH v5 5/5] udp: Update UDP " Laurent Vivier
2024-09-30 2:58 ` David Gibson
2024-10-02 14:32 ` [PATCH v5 0/5] tcp: use csum_iov() in tcp_update_check_tcp[4|6]() Stefano Brivio
2024-10-02 14:46 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241001202216.5c3ee274@elisabeth \
--to=sbrivio@redhat.com \
--cc=lvivier@redhat.com \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).