| Message ID | 20240226133837.533820-1-mkp@redhat.com |
|---|---|
| State | Accepted |
| Commit | 4c32b6d0964c09396db94bf2f3021f02cdcfd48e |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] dp-packet: Don't offload inner csum if outer isn't supported. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 2/26/24 14:38, Mike Pattrick wrote: > Some network cards support inner checksum offloading but not outer > checksum offloading. Currently OVS will resolve that outer checksum but > allows the network card to resolve the inner checksum, invalidating the > outer checksum in the process. > > Now if we can't offload outer checksums, we don't offload inner either. > > Reported-at: https://issues.redhat.com/browse/FDP-363 > Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > nb: I also tested a more complex patch that only resolved the inner > checksum and offloaded the UDP layer. This didn't noticably improve > performance. > v2: Added IPv4 flag > --- > lib/dp-packet.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index 305822293..df7bf8e6b 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -592,6 +592,18 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags) > if (dp_packet_hwol_is_tunnel_geneve(p) || > dp_packet_hwol_is_tunnel_vxlan(p)) { > tnl_inner = true; > + > + /* If the TX interface doesn't support UDP tunnel offload but does > + * support inner checksum offload and an outer UDP checksum is > + * required, then we can't offload inner checksum either. As that would > + * invalidate the outer checksum. */ > + if (!(flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM) && > + dp_packet_hwol_is_outer_udp_cksum(p)) { > + flags &= ~(NETDEV_TX_OFFLOAD_TCP_CKSUM | > + NETDEV_TX_OFFLOAD_UDP_CKSUM | > + NETDEV_TX_OFFLOAD_SCTP_CKSUM | > + NETDEV_TX_OFFLOAD_IPV4_CKSUM); Thanks, Mike. Applied and backported to 3.3. Though I still think the logic of offload flags is backwards. Unfortunately that is derivative of the kernel and DPDK APIs. Maybe we can try to flip it in OVS? I think it might be good if we could make all the basic flags always mean offloads of the outermost headers and have another set of inner flags. Maybe we could bitshift them on encapsulations/decapsulations. This will require flag translation on receive and send for each particular netdev type. But maybe it will be more clear to implement OVS logic this way, as long as it doesn't give a huge performance hit. I'd happily sacrifice a couple percents of performance for a clearer internal logic. Any other ideas are also welcome. But the fact that basic flags mean innermost header, except for cases where they do not, doesn't make a lot of sense to me and we'll keep hitting issues like this one because of that. Best regards, Ilya Maximets.
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 305822293..df7bf8e6b 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -592,6 +592,18 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags) if (dp_packet_hwol_is_tunnel_geneve(p) || dp_packet_hwol_is_tunnel_vxlan(p)) { tnl_inner = true; + + /* If the TX interface doesn't support UDP tunnel offload but does + * support inner checksum offload and an outer UDP checksum is + * required, then we can't offload inner checksum either. As that would + * invalidate the outer checksum. */ + if (!(flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM) && + dp_packet_hwol_is_outer_udp_cksum(p)) { + flags &= ~(NETDEV_TX_OFFLOAD_TCP_CKSUM | + NETDEV_TX_OFFLOAD_UDP_CKSUM | + NETDEV_TX_OFFLOAD_SCTP_CKSUM | + NETDEV_TX_OFFLOAD_IPV4_CKSUM); + } } if (dp_packet_hwol_tx_ip_csum(p)) {
Some network cards support inner checksum offloading but not outer checksum offloading. Currently OVS will resolve that outer checksum but allows the network card to resolve the inner checksum, invalidating the outer checksum in the process. Now if we can't offload outer checksums, we don't offload inner either. Reported-at: https://issues.redhat.com/browse/FDP-363 Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") Signed-off-by: Mike Pattrick <mkp@redhat.com> --- nb: I also tested a more complex patch that only resolved the inner checksum and offloaded the UDP layer. This didn't noticably improve performance. v2: Added IPv4 flag --- lib/dp-packet.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)