Hi Konstantin,


On 9/19/2016 8:09 PM, Ananyev, Konstantin wrote:

Hi Jainfeng,

quoted

-----Original Message-----
From: Tan, Jianfeng
Sent: Monday, August 1, 2016 4:57 AM
To: dev@dpdk.org
Cc: thomas.monjalon@6wind.com; De Lara Guarch, Pablo <redacted>; Ananyev, Konstantin
[off-list ref]; Wu, Jingjing [off-list ref]; Zhang, Helin [off-list ref]; Tan, Jianfeng
[off-list ref]; Tao, Zhe [off-list ref]
Subject: [PATCH v4 3/3] app/testpmd: fix Tx offload on tunneling packet

Tx offload on tunneling packet now requires applications to correctly set tunneling type. Without setting it, i40e driver does not parse
tunneling parameters. Besides that, add a check to see if NIC supports TSO on tunneling packet when executing "csum parse_tunnel on
_port"
after "tso set _size _port" or the other way around.

Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward engine")

Signed-off-by: Zhe Tao <redacted>
Signed-off-by: Jianfeng Tan <redacted>
---
  app/test-pmd/cmdline.c  | 42 ++++++++++++++++++++++++++++++++++++------
  app/test-pmd/csumonly.c | 37 +++++++++++++++++++++++++++++--------
  2 files changed, 65 insertions(+), 14 deletions(-)

[...]

@@ -745,7 +762,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
  		 * processed in hardware. */
  		if (info.is_tunnel == 1) {
  			ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
-				testpmd_ol_flags);
+				testpmd_ol_flags, ol_flags & PKT_TX_TCP_SEG);
  		}

  		/* step 4: fill the mbuf meta data (flags and header lengths) */ @@ -806,6 +823,10 @@

It was a while since I looked a t it closely, but shouldn't you also update step 4 below:

if (info.is_tunnel == 1) {
                         if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) {
                                 m->outer_l2_len = info.outer_l2_len;
                                 m->outer_l3_len = info.outer_l3_len;
                                 m->l2_len = info.l2_len;
                                 m->l3_len = info.l3_len;
                                 m->l4_len = info.l4_len;
                         }
                         else {
                                 /* if there is a outer UDP cksum
                                    processed in sw and the inner in hw,
                                    the outer checksum will be wrong as
                                    the payload will be modified by the
                                    hardware */
                                 m->l2_len = info.outer_l2_len +
                                         info.outer_l3_len + info.l2_len;
                                 m->l3_len = info.l3_len;
                                 m->l4_len = info.l4_len;
                         }


?

In particular shouldn't it be something like:
if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) != 0 ||
       ((testmpd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) != 0 && info.tso_segsz != 0)) {
....
?

Sorry for late response, because I also take some time to refresh 
memory. And, you are right, I missed this corner case. After applying 
your way above, it works!

The case below settings in testpmd:
$ set fwd csum
$ csum parse_tunnel on 0
$ tso set 800 0
<keep outer-ip checksum offload is sw>

And unfortunately, our previous verification is based on "outer-ip 
checksum offload is hw".

Another thought, might be it is worth to introduce new flag: TESTPMD_TX_OFFLOAD_TSO_TUNNEL,
and new command in cmdline.c, that would set/clear that flag.
Instead of trying to make assumptions does
user wants tso for tunneled packets based on 2 different things:
- enable/disable tso
- enable/disable tunneled packets parsing
?

Currently, if we do parse_tunnel is based on the command "csum 
parse_tunnel on/off <port>".
If we add a command like "tso_tunnel set <length> <port>", it's a little 
duplicated with "tso set <length> <port>", and there is too much info to 
just set a flag like TESTPMD_TX_OFFLOAD_TSO_TUNNEL;
If we add a command like "csum tunnel_tso on <port>", it also depends on 
"csum parse_tunnel on <port>" so that tunnel packets are parsed.

As far as I can see, the new command will always have semantic 
overlapping with existing commands, because it indeed depends on the two 
different things.

Thanks,
Jianfeng

Konstantin