Re: [PATCH v4 3/3] app/testpmd: fix Tx offload on tunneling packet

Hi Konstantin,


On 9/21/2016 11:47 PM, Ananyev, Konstantin wrote:

Hi Jianfeng,

quoted

Hi Konstantin,


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

quoted

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
[off-list ref]; 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"

quoted

quoted

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>

Great :)

quoted

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

quoted

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.

But I thought in some cases user might want to enable tunnel parsing, but do tso for non-tunneled packets only.
I.E.
  - enable tunnel parsing
- for non-tunneled packets do tso
- for tunneled packets don't do tso
My understanding that with current set commands/flags this is not possible, correct?
Konstantin

Yes, correct, above case is not supported now. A twin case would be:
- for non-tunneled packets, don't do tso
- for tunneled packets, do tso

Considering above two cases, so how about adding a command like;
$ tunnel_tso set 800 0
which needs "csum parse_tunnel on 0" has been set before it.

And original "tso set 800 0" will only control tso of non-tunneled packets.
?


Thanks,
Jianfeng