[dpdk-dev,v4,3/3] app/testpmd: fix Tx offload on tunneling packet
Commit Message
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 <zhe.tao@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++------
app/test-pmd/csumonly.c | 37 +++++++++++++++++++++++++++++--------
2 files changed, 65 insertions(+), 14 deletions(-)
Comments
Hi Jainfeng,
> -----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 <pablo.de.lara.guarch@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Tan, Jianfeng
> <jianfeng.tan@intel.com>; Tao, Zhe <zhe.tao@intel.com>
> 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 <zhe.tao@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++------
> app/test-pmd/csumonly.c | 37 +++++++++++++++++++++++++++++--------
> 2 files changed, 65 insertions(+), 14 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index f90befc..561839f 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -3426,6 +3426,26 @@ struct cmd_csum_tunnel_result { };
>
> static void
> +check_tunnel_tso_support(uint8_t port_id) {
> + struct rte_eth_dev_info dev_info;
> +
> + rte_eth_dev_info_get(port_id, &dev_info);
> + if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VXLAN_TNL_TSO))
> + printf("Warning: TSO enabled but VXLAN TUNNEL TSO not "
> + "supported by port %d\n", port_id);
> + if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GRE_TNL_TSO))
> + printf("Warning: TSO enabled but GRE TUNNEL TSO not "
> + "supported by port %d\n", port_id);
> + if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPIP_TNL_TSO))
> + printf("Warning: TSO enabled but IPIP TUNNEL TSO not "
> + "supported by port %d\n", port_id);
> + if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO))
> + printf("Warning: TSO enabled but GENEVE TUNNEL TSO not "
> + "supported by port %d\n", port_id); }
> +
> +static void
> cmd_csum_tunnel_parsed(void *parsed_result,
> __attribute__((unused)) struct cmdline *cl,
> __attribute__((unused)) void *data) @@ -3435,10 +3455,13 @@ cmd_csum_tunnel_parsed(void *parsed_result,
> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> return;
>
> - if (!strcmp(res->onoff, "on"))
> + if (!strcmp(res->onoff, "on")) {
> ports[res->port_id].tx_ol_flags |=
> TESTPMD_TX_OFFLOAD_PARSE_TUNNEL;
> - else
> +
> + if (ports[res->port_id].tso_segsz != 0)
> + check_tunnel_tso_support(res->port_id);
> + } else
> ports[res->port_id].tx_ol_flags &=
> (~TESTPMD_TX_OFFLOAD_PARSE_TUNNEL);
>
> @@ -3502,10 +3525,17 @@ cmd_tso_set_parsed(void *parsed_result,
>
> /* display warnings if configuration is not supported by the NIC */
> rte_eth_dev_info_get(res->port_id, &dev_info);
> - if ((ports[res->port_id].tso_segsz != 0) &&
> - (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO) == 0) {
> - printf("Warning: TSO enabled but not "
> - "supported by port %d\n", res->port_id);
> + if (ports[res->port_id].tso_segsz != 0) {
> + if (ports[res->port_id].tx_ol_flags &
> + TESTPMD_TX_OFFLOAD_PARSE_TUNNEL)
> + check_tunnel_tso_support(res->port_id);
> + /* For packets,
> + * (1) when tnl parse is disabled;
> + * (2) when tnl parse is enabled but not deemed as tnl pkts
> + */
> + if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO))
> + printf("Warning: TSO enabled but not "
> + "supported by port %d\n", res->port_id);
> }
> }
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index ac4bd8f..0a1f95d 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -412,12 +412,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> return ol_flags;
> }
>
> -/* Calculate the checksum of outer header (only vxlan is supported,
> - * meaning IP + UDP). The caller already checked that it's a vxlan
> - * packet */
> +/* Calculate the checksum of outer header */
> static uint64_t
> process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
> - uint16_t testpmd_ol_flags)
> + uint16_t testpmd_ol_flags, int tso_enabled)
> {
> struct ipv4_hdr *ipv4_hdr = outer_l3_hdr;
> struct ipv6_hdr *ipv6_hdr = outer_l3_hdr; @@ -438,10 +436,20 @@ process_outer_cksums(void *outer_l3_hdr, struct
> testpmd_offload_info *info,
> if (info->outer_l4_proto != IPPROTO_UDP)
> return ol_flags;
>
> - /* outer UDP checksum is always done in software as we have no
> - * hardware supporting it today, and no API for it. */
> -
> udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + info->outer_l3_len);
> +
> + /* outer UDP checksum is done in software as we have no hardware
> + * supporting it today, and no API for it. In the other side, for
> + * UDP tunneling, like VXLAN or Geneve, outer UDP checksum can be
> + * set to zero.
> + *
> + * If a packet will be TSOed into small packets by NIC, we cannot
> + * set/calculate a non-zero checksum, because it will be a wrong
> + * value after the packet be split into several small packets.
> + */
> + if (tso_enabled)
> + udp_hdr->dgram_cksum = 0;
> +
> /* do not recalculate udp cksum if it was 0 */
> if (udp_hdr->dgram_cksum != 0) {
> udp_hdr->dgram_cksum = 0;
> @@ -704,18 +712,27 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) {
> if (info.l4_proto == IPPROTO_UDP) {
> struct udp_hdr *udp_hdr;
> +
> udp_hdr = (struct udp_hdr *)((char *)l3_hdr +
> info.l3_len);
> parse_vxlan(udp_hdr, &info, m->packet_type);
> + if (info.is_tunnel)
> + ol_flags |= PKT_TX_TUNNEL_VXLAN;
> } else if (info.l4_proto == IPPROTO_GRE) {
> struct simple_gre_hdr *gre_hdr;
> +
> gre_hdr = (struct simple_gre_hdr *)
> ((char *)l3_hdr + info.l3_len);
> parse_gre(gre_hdr, &info);
> + if (info.is_tunnel)
> + ol_flags |= PKT_TX_TUNNEL_GRE;
> } else if (info.l4_proto == IPPROTO_IPIP) {
> void *encap_ip_hdr;
> +
> encap_ip_hdr = (char *)l3_hdr + info.l3_len;
> parse_encap_ip(encap_ip_hdr, &info);
> + if (info.is_tunnel)
> + ol_flags |= PKT_TX_TUNNEL_IPIP;
> }
> }
>
> @@ -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)) {
....
?
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
?
Konstantin
> pkt_burst_checksum_forward(struct fwd_stream *fs)
> { PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
> { PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
> { PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
> + { PKT_TX_TUNNEL_VXLAN, PKT_TX_TUNNEL_MASK },
> + { PKT_TX_TUNNEL_GRE, PKT_TX_TUNNEL_MASK },
> + { PKT_TX_TUNNEL_IPIP, PKT_TX_TUNNEL_MASK },
> + { PKT_TX_TUNNEL_GENEVE, PKT_TX_TUNNEL_MASK },
> };
> unsigned j;
> const char *name;
> --
> 2.7.4
Hi Konstantin,
On 9/19/2016 8:09 PM, Ananyev, Konstantin wrote:
> Hi Jainfeng,
>
>> -----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 <pablo.de.lara.guarch@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Tan, Jianfeng
>> <jianfeng.tan@intel.com>; Tao, Zhe <zhe.tao@intel.com>
>> 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 <zhe.tao@intel.com>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>> 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
>
Hi Jianfeng,
>
> Hi Konstantin,
>
>
> On 9/19/2016 8:09 PM, Ananyev, Konstantin wrote:
> > Hi Jainfeng,
> >
> >> -----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
> >> <pablo.de.lara.guarch@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> >> Zhang, Helin <helin.zhang@intel.com>; Tan, Jianfeng
> >> <jianfeng.tan@intel.com>; Tao, Zhe <zhe.tao@intel.com>
> >> 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 <zhe.tao@intel.com>
> >> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >> ---
> >> 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 :)
>
> 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.
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
>
> 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
> >
Hi Konstantin,
On 9/21/2016 11:47 PM, Ananyev, Konstantin wrote:
> Hi Jianfeng,
>
>> Hi Konstantin,
>>
>>
>> On 9/19/2016 8:09 PM, Ananyev, Konstantin wrote:
>>> Hi Jainfeng,
>>>
>>>> -----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
>>>> <pablo.de.lara.guarch@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
>>>> Zhang, Helin <helin.zhang@intel.com>; Tan, Jianfeng
>>>> <jianfeng.tan@intel.com>; Tao, Zhe <zhe.tao@intel.com>
>>>> 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 <zhe.tao@intel.com>
>>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>>> ---
>>>> 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 :)
>
>> 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.
> 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
Hi Jianfeng,
>
> Hi Konstantin,
>
>
> On 9/21/2016 11:47 PM, Ananyev, Konstantin wrote:
> > Hi Jianfeng,
> >
> >> Hi Konstantin,
> >>
> >>
> >> On 9/19/2016 8:09 PM, Ananyev, Konstantin wrote:
> >>> Hi Jainfeng,
> >>>
> >>>> -----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
> >>>> <pablo.de.lara.guarch@intel.com>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Wu, Jingjing
> >>>> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Tan,
> >>>> Jianfeng <jianfeng.tan@intel.com>; Tao, Zhe <zhe.tao@intel.com>
> >>>> 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 <zhe.tao@intel.com>
> >>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >>>> ---
> >>>> 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 :)
> >
> >> 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.
> > 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
Yep, you right.
>
> 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.
Looks good for me.
Konstantin
@@ -3426,6 +3426,26 @@ struct cmd_csum_tunnel_result {
};
static void
+check_tunnel_tso_support(uint8_t port_id)
+{
+ struct rte_eth_dev_info dev_info;
+
+ rte_eth_dev_info_get(port_id, &dev_info);
+ if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VXLAN_TNL_TSO))
+ printf("Warning: TSO enabled but VXLAN TUNNEL TSO not "
+ "supported by port %d\n", port_id);
+ if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GRE_TNL_TSO))
+ printf("Warning: TSO enabled but GRE TUNNEL TSO not "
+ "supported by port %d\n", port_id);
+ if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPIP_TNL_TSO))
+ printf("Warning: TSO enabled but IPIP TUNNEL TSO not "
+ "supported by port %d\n", port_id);
+ if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO))
+ printf("Warning: TSO enabled but GENEVE TUNNEL TSO not "
+ "supported by port %d\n", port_id);
+}
+
+static void
cmd_csum_tunnel_parsed(void *parsed_result,
__attribute__((unused)) struct cmdline *cl,
__attribute__((unused)) void *data)
@@ -3435,10 +3455,13 @@ cmd_csum_tunnel_parsed(void *parsed_result,
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
return;
- if (!strcmp(res->onoff, "on"))
+ if (!strcmp(res->onoff, "on")) {
ports[res->port_id].tx_ol_flags |=
TESTPMD_TX_OFFLOAD_PARSE_TUNNEL;
- else
+
+ if (ports[res->port_id].tso_segsz != 0)
+ check_tunnel_tso_support(res->port_id);
+ } else
ports[res->port_id].tx_ol_flags &=
(~TESTPMD_TX_OFFLOAD_PARSE_TUNNEL);
@@ -3502,10 +3525,17 @@ cmd_tso_set_parsed(void *parsed_result,
/* display warnings if configuration is not supported by the NIC */
rte_eth_dev_info_get(res->port_id, &dev_info);
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO) == 0) {
- printf("Warning: TSO enabled but not "
- "supported by port %d\n", res->port_id);
+ if (ports[res->port_id].tso_segsz != 0) {
+ if (ports[res->port_id].tx_ol_flags &
+ TESTPMD_TX_OFFLOAD_PARSE_TUNNEL)
+ check_tunnel_tso_support(res->port_id);
+ /* For packets,
+ * (1) when tnl parse is disabled;
+ * (2) when tnl parse is enabled but not deemed as tnl pkts
+ */
+ if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO))
+ printf("Warning: TSO enabled but not "
+ "supported by port %d\n", res->port_id);
}
}
@@ -412,12 +412,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
return ol_flags;
}
-/* Calculate the checksum of outer header (only vxlan is supported,
- * meaning IP + UDP). The caller already checked that it's a vxlan
- * packet */
+/* Calculate the checksum of outer header */
static uint64_t
process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
- uint16_t testpmd_ol_flags)
+ uint16_t testpmd_ol_flags, int tso_enabled)
{
struct ipv4_hdr *ipv4_hdr = outer_l3_hdr;
struct ipv6_hdr *ipv6_hdr = outer_l3_hdr;
@@ -438,10 +436,20 @@ process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
if (info->outer_l4_proto != IPPROTO_UDP)
return ol_flags;
- /* outer UDP checksum is always done in software as we have no
- * hardware supporting it today, and no API for it. */
-
udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + info->outer_l3_len);
+
+ /* outer UDP checksum is done in software as we have no hardware
+ * supporting it today, and no API for it. In the other side, for
+ * UDP tunneling, like VXLAN or Geneve, outer UDP checksum can be
+ * set to zero.
+ *
+ * If a packet will be TSOed into small packets by NIC, we cannot
+ * set/calculate a non-zero checksum, because it will be a wrong
+ * value after the packet be split into several small packets.
+ */
+ if (tso_enabled)
+ udp_hdr->dgram_cksum = 0;
+
/* do not recalculate udp cksum if it was 0 */
if (udp_hdr->dgram_cksum != 0) {
udp_hdr->dgram_cksum = 0;
@@ -704,18 +712,27 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) {
if (info.l4_proto == IPPROTO_UDP) {
struct udp_hdr *udp_hdr;
+
udp_hdr = (struct udp_hdr *)((char *)l3_hdr +
info.l3_len);
parse_vxlan(udp_hdr, &info, m->packet_type);
+ if (info.is_tunnel)
+ ol_flags |= PKT_TX_TUNNEL_VXLAN;
} else if (info.l4_proto == IPPROTO_GRE) {
struct simple_gre_hdr *gre_hdr;
+
gre_hdr = (struct simple_gre_hdr *)
((char *)l3_hdr + info.l3_len);
parse_gre(gre_hdr, &info);
+ if (info.is_tunnel)
+ ol_flags |= PKT_TX_TUNNEL_GRE;
} else if (info.l4_proto == IPPROTO_IPIP) {
void *encap_ip_hdr;
+
encap_ip_hdr = (char *)l3_hdr + info.l3_len;
parse_encap_ip(encap_ip_hdr, &info);
+ if (info.is_tunnel)
+ ol_flags |= PKT_TX_TUNNEL_IPIP;
}
}
@@ -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 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
{ PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
+ { PKT_TX_TUNNEL_VXLAN, PKT_TX_TUNNEL_MASK },
+ { PKT_TX_TUNNEL_GRE, PKT_TX_TUNNEL_MASK },
+ { PKT_TX_TUNNEL_IPIP, PKT_TX_TUNNEL_MASK },
+ { PKT_TX_TUNNEL_GENEVE, PKT_TX_TUNNEL_MASK },
};
unsigned j;
const char *name;