Message ID | 20211123162211.267094-1-jun.xiao@cloudnetengine.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev] packets: fix l4 pseudo csum incremental update | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Jun, the changes look good, but some small comments below. //Eelco On 23 Nov 2021, at 17:22, Jun Xiao wrote: > When a packet requests l4 csum offload, the l4 csum field includes > a pseudo csum instead of a full csum. The pseudo csum doesn't do > the final one's complement, and it must be updated differently > than a full csum. > > Signed-off-by: Jun Xiao <jun.xiao@cloudnetengine.com> > --- > lib/packets.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/lib/packets.c b/lib/packets.c > index 4a7643c5d..910d14379 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -1088,11 +1088,17 @@ packet_set_ipv4_addr(struct dp_packet *packet, > struct tcp_header *th = dp_packet_l4(packet); > > th->tcp_csum = recalc_csum32(th->tcp_csum, old_addr, new_addr); > + if (dp_packet_hwol_tx_l4_checksum(packet)) { I would use the more specific tcp version of the function, dp_packet_hwol_l4_is_tcp() > + th->tcp_csum = ~th->tcp_csum; > + } > } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN ) { > struct udp_header *uh = dp_packet_l4(packet); > > if (uh->udp_csum) { > uh->udp_csum = recalc_csum32(uh->udp_csum, old_addr, new_addr); > + if (dp_packet_hwol_tx_l4_checksum(packet)) { I would use the more specific udp version of the function, dp_packet_hwol_l4_is_udp() > + uh->udp_csum = ~uh->udp_csum; > + } > if (!uh->udp_csum) { > uh->udp_csum = htons(0xffff); > } > @@ -1196,11 +1202,17 @@ packet_update_csum128(struct dp_packet *packet, uint8_t proto, > struct tcp_header *th = dp_packet_l4(packet); > > th->tcp_csum = recalc_csum128(th->tcp_csum, addr, new_addr); > + if (dp_packet_hwol_tx_l4_checksum(packet)) { > + th->tcp_csum = ~th->tcp_csum; I would use the more specific tcp version of the function, dp_packet_hwol_l4_is_tcp() > + } > } else if (proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) { > struct udp_header *uh = dp_packet_l4(packet); > > if (uh->udp_csum) { > uh->udp_csum = recalc_csum128(uh->udp_csum, addr, new_addr); > + if (dp_packet_hwol_tx_l4_checksum(packet)) { I would use the more specific udp version of the function, dp_packet_hwol_l4_is_udp() > + uh->udp_csum = ~uh->udp_csum; > + } > if (!uh->udp_csum) { > uh->udp_csum = htons(0xffff); Is this case also true for the offload case? > } > -- > 2.25.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/packets.c b/lib/packets.c index 4a7643c5d..910d14379 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -1088,11 +1088,17 @@ packet_set_ipv4_addr(struct dp_packet *packet, struct tcp_header *th = dp_packet_l4(packet); th->tcp_csum = recalc_csum32(th->tcp_csum, old_addr, new_addr); + if (dp_packet_hwol_tx_l4_checksum(packet)) { + th->tcp_csum = ~th->tcp_csum; + } } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN ) { struct udp_header *uh = dp_packet_l4(packet); if (uh->udp_csum) { uh->udp_csum = recalc_csum32(uh->udp_csum, old_addr, new_addr); + if (dp_packet_hwol_tx_l4_checksum(packet)) { + uh->udp_csum = ~uh->udp_csum; + } if (!uh->udp_csum) { uh->udp_csum = htons(0xffff); } @@ -1196,11 +1202,17 @@ packet_update_csum128(struct dp_packet *packet, uint8_t proto, struct tcp_header *th = dp_packet_l4(packet); th->tcp_csum = recalc_csum128(th->tcp_csum, addr, new_addr); + if (dp_packet_hwol_tx_l4_checksum(packet)) { + th->tcp_csum = ~th->tcp_csum; + } } else if (proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) { struct udp_header *uh = dp_packet_l4(packet); if (uh->udp_csum) { uh->udp_csum = recalc_csum128(uh->udp_csum, addr, new_addr); + if (dp_packet_hwol_tx_l4_checksum(packet)) { + uh->udp_csum = ~uh->udp_csum; + } if (!uh->udp_csum) { uh->udp_csum = htons(0xffff); }
When a packet requests l4 csum offload, the l4 csum field includes a pseudo csum instead of a full csum. The pseudo csum doesn't do the final one's complement, and it must be updated differently than a full csum. Signed-off-by: Jun Xiao <jun.xiao@cloudnetengine.com> --- lib/packets.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)