Thread (21 messages) 21 messages, 7 authors, 2021-08-02

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix TX checksum calculation for tunnel

From: Gregory Etelson <hidden>
Date: 2021-07-29 10:31:55

Hello Olivier,

[:snip:]
quoted
Correct. Inner checksum is offloaded and outer computed in software.
I think this approach is not sane: the value of the outer checksum depends on
the inner checksum, so it has to be calculated after. There is a comment in the
code about this:

        /* Then process outer headers if any. Note that the software
         * checksum will be wrong if one of the inner checksums is
         * processed in hardware. */
        if (info.is_tunnel == 1) {
                tx_ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
                                tx_offloads,
                                !!(tx_ol_flags & PKT_TX_TCP_SEG));
        }
I agree. That computation order led to error in my case.
What if the engine could analyze port TX offload options and select 
processing order according to existing configuration ?
 
quoted
Consider this example:
Tunneled packet arrived at port A and being forwarded through port B.
The packet arrived at port A with correct inner checksums - L3 and L4.
Port B TX offloads inner L3 only.

process_inner_cksums() sets "ipv4_hdr->hdr_checksum = 0;"
unconditionally.
quoted
Inner L3 checksum value will be restored by port B TX checksum
offload, but when
process_outer_cksums() runs software calculation on outer L4 it will use 0
and produce wrong result.
quoted
Therefore, the patch zeros inner checksum values only before actual
software calculations.

I better understand your use case, thanks.

However, with your patch, if the inner L4 checksum is wrong when it arrives
on port A, I think it will result in a packet with a wrong outer
L4 checksum and a correct inner L4 checksum. Is it what you expect?
Wrong checksum reflects ongoing issue that must be investigated and fixed.
I don't expect forwarding engine to fix wrong checksum because it can hide
a real problem.
I don't argue against the patch itself. What you suggest better matches the
offload API than what we have today. Can you please send another version
that better explains the use-case?
I posted v3 with updated comment.
One more suggestion, maybe for later. Currently, the csumonly engine can be
configured to do the checksum in sw or in hw. Maybe we could add a "dont-
touch" option, to keep the value in the packet. Would it help for your use-
case?
That's a very good idea.
Packets can arrive with pre-calculated correct checksums. Keeping these values
can save processing time.

[:snip:]

Regards,
Gregory
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help