Messages in this thread | | | Date | Tue, 30 Apr 2024 12:57:33 +0200 | Subject | Re: [PATCH v4 net-next v4 2/6] net: add support for segmenting TCP fraglist GSO packets | From | Felix Fietkau <> |
| |
On 30.04.24 12:40, Paolo Abeni wrote: > On Tue, 2024-04-30 at 12:27 +0200, Felix Fietkau wrote: >> On 30.04.24 12:19, Paolo Abeni wrote: >> > On Sat, 2024-04-27 at 20:22 +0200, Felix Fietkau wrote: >> > > Preparation for adding TCP fraglist GRO support. It expects packets to be >> > > combined in a similar way as UDP fraglist GSO packets. >> > > For IPv4 packets, NAT is handled in the same way as UDP fraglist GSO. >> > > >> > > Signed-off-by: Felix Fietkau <nbd@nbd.name> >> > > --- >> > > net/ipv4/tcp_offload.c | 67 ++++++++++++++++++++++++++++++++++++++++ >> > > net/ipv6/tcpv6_offload.c | 58 ++++++++++++++++++++++++++++++++++ >> > > 2 files changed, 125 insertions(+) >> > > >> > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c >> > > index fab0973f995b..affd4ed28cfe 100644 >> > > --- a/net/ipv4/tcp_offload.c >> > > +++ b/net/ipv4/tcp_offload.c >> > > @@ -28,6 +28,70 @@ static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq, >> > > } >> > > } >> > > >> > > +static void __tcpv4_gso_segment_csum(struct sk_buff *seg, >> > > + __be32 *oldip, __be32 newip, >> > > + __be16 *oldport, __be16 newport) >> > > +{ >> > > + struct tcphdr *th; >> > > + struct iphdr *iph; >> > > + >> > > + if (*oldip == newip && *oldport == newport) >> > > + return; >> > > + >> > > + th = tcp_hdr(seg); >> > > + iph = ip_hdr(seg); >> > > + >> > > + inet_proto_csum_replace4(&th->check, seg, *oldip, newip, true); >> > > + inet_proto_csum_replace2(&th->check, seg, *oldport, newport, false); >> > > + *oldport = newport; >> > > + >> > > + csum_replace4(&iph->check, *oldip, newip); >> > > + *oldip = newip; >> > > +} >> > > + >> > > +static struct sk_buff *__tcpv4_gso_segment_list_csum(struct sk_buff *segs) >> > > +{ >> > > + const struct tcphdr *th; >> > > + const struct iphdr *iph; >> > > + struct sk_buff *seg; >> > > + struct tcphdr *th2; >> > > + struct iphdr *iph2; >> > > + >> > > + seg = segs; >> > > + th = tcp_hdr(seg); >> > > + iph = ip_hdr(seg); >> > > + th2 = tcp_hdr(seg->next); >> > > + iph2 = ip_hdr(seg->next); >> > > + >> > > + if (!(*(const u32 *)&th->source ^ *(const u32 *)&th2->source) && >> > > + iph->daddr == iph2->daddr && iph->saddr == iph2->saddr) >> > > + return segs; >> > > + >> > > + while ((seg = seg->next)) { >> > > + th2 = tcp_hdr(seg); >> > > + iph2 = ip_hdr(seg); >> > > + >> > > + __tcpv4_gso_segment_csum(seg, >> > > + &iph2->saddr, iph->saddr, >> > > + &th2->source, th->source); >> > > + __tcpv4_gso_segment_csum(seg, >> > > + &iph2->daddr, iph->daddr, >> > > + &th2->dest, th->dest); >> > > + } >> > > + >> > > + return segs; >> > > +} >> > >> > AFAICS, all the above is really alike the UDP side, except for the >> > transport header zero csum. >> > >> > What about renaming the udp version of this helpers as 'tcpudpv4_...', >> > move them in common code, add an explicit argument for >> > 'zerocsum_allowed' and reuse such helper for both tcp and udp? >> > >> > The same for the ipv6 variant. >> >> Wouldn't that make it more convoluted when taking into account that the >> checksum field offset is different for tcp vs udp? >> How would you handle that? > > Probably making a common helper just for > __tcpudpv{4,6}_gso_segment_csum and pass it the target l4 csum pointer > as an additional argument. It would not be spectacularly nice, so no > strong opinion either way.
I'd rather keep it duplicated but more straightforward and easier to read.
- Felix
| |