* [PATCH] app/testpmd: fix UDP cksum error for UFO enable
@ 2023-07-28 2:13 Huisong Li
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
0 siblings, 2 replies; 14+ messages in thread
From: Huisong Li @ 2023-07-28 2:13 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, aman.deep.singh, yuying.zhang,
zhichaox.zeng, lihuisong
The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
tso_segsz is set. Then tx_prepare() may call rte_net_intel_cksum_prepare()
to compute pseudo header checksum (because some PMDs may supports TSO).
As a result, if the peer sends UDP packets, all packets with UDP checksum
error are received for the PMDs only supported TSO.
So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
capability. Similarly, TSO also need to do like this.
In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
it better to support TSO and UFO.
Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
app/test-pmd/csumonly.c | 4 ++--
2 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0d0723f659..8be593d405 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
{
struct cmd_tso_set_result *res = parsed_result;
struct rte_eth_dev_info dev_info;
+ uint64_t offloads;
int ret;
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
@@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
if (ret != 0)
return;
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr, "Error: TSO is not supported by port %d\n",
- res->port_id);
- return;
+ if (ports[res->port_id].tso_segsz != 0) {
+ if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
+ RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
+ fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
+ res->port_id);
+ return;
+ }
+ /* display warnings if configuration is not supported by the NIC */
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
+ fprintf(stderr, "Warning: port %d doesn't support TSO\n",
+ res->port_id);
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
+ fprintf(stderr, "Warning: port %d doesn't support UFO\n",
+ res->port_id);
}
if (ports[res->port_id].tso_segsz == 0) {
ports[res->port_id].dev_conf.txmode.offloads &=
- ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO for non-tunneled packets is disabled\n");
+ ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
+ printf("TSO and UFO for non-tunneled packets is disabled\n");
} else {
- ports[res->port_id].dev_conf.txmode.offloads |=
- RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO segment size for non-tunneled packets is %d\n",
+ offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
+ offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
+ ports[res->port_id].dev_conf.txmode.offloads |= offloads;
+ printf("segment size for non-tunneled packets is %d\n",
ports[res->port_id].tso_segsz);
}
- cmd_config_queue_tx_offloads(&ports[res->port_id]);
-
- /* display warnings if configuration is not supported by the NIC */
- ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
- if (ret != 0)
- return;
-
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr,
- "Warning: TSO enabled but not supported by port %d\n",
- res->port_id);
- }
+ cmd_config_queue_tx_offloads(&ports[res->port_id]);
cmd_reconfig_device_queue(res->port_id, 1, 1);
}
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..0c007bd178 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -505,7 +505,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
/* do not recalculate udp cksum if it was 0 */
if (udp_hdr->dgram_cksum != 0) {
- if (tso_segsz)
+ if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
@@ -528,7 +528,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
#endif
} else if (info->l4_proto == IPPROTO_TCP) {
tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
- if (tso_segsz)
+ if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_TSO))
ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
--
2.33.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-07-28 2:13 [PATCH] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
@ 2023-08-02 2:55 ` Huisong Li
2023-10-20 3:38 ` lihuisong (C)
` (2 more replies)
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
1 sibling, 3 replies; 14+ messages in thread
From: Huisong Li @ 2023-08-02 2:55 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, aman.deep.singh, yuying.zhang,
zhichaox.zeng, lihuisong
The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
tso_segsz is set. Then tx_prepare() may call rte_net_intel_cksum_prepare()
to compute pseudo header checksum (because some PMDs may supports TSO).
As a result, if the peer sends UDP packets, all packets with UDP checksum
error are received for the PMDs only supported TSO.
So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
capability. Similarly, TSO also need to do like this.
In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
it better to support TSO and UFO.
Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
v2: add handle for tunnel TSO offload in process_inner_cksums
---
app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
app/test-pmd/csumonly.c | 11 ++++++++--
2 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0d0723f659..8be593d405 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
{
struct cmd_tso_set_result *res = parsed_result;
struct rte_eth_dev_info dev_info;
+ uint64_t offloads;
int ret;
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
@@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
if (ret != 0)
return;
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr, "Error: TSO is not supported by port %d\n",
- res->port_id);
- return;
+ if (ports[res->port_id].tso_segsz != 0) {
+ if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
+ RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
+ fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
+ res->port_id);
+ return;
+ }
+ /* display warnings if configuration is not supported by the NIC */
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
+ fprintf(stderr, "Warning: port %d doesn't support TSO\n",
+ res->port_id);
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
+ fprintf(stderr, "Warning: port %d doesn't support UFO\n",
+ res->port_id);
}
if (ports[res->port_id].tso_segsz == 0) {
ports[res->port_id].dev_conf.txmode.offloads &=
- ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO for non-tunneled packets is disabled\n");
+ ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
+ printf("TSO and UFO for non-tunneled packets is disabled\n");
} else {
- ports[res->port_id].dev_conf.txmode.offloads |=
- RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO segment size for non-tunneled packets is %d\n",
+ offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
+ offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
+ ports[res->port_id].dev_conf.txmode.offloads |= offloads;
+ printf("segment size for non-tunneled packets is %d\n",
ports[res->port_id].tso_segsz);
}
- cmd_config_queue_tx_offloads(&ports[res->port_id]);
-
- /* display warnings if configuration is not supported by the NIC */
- ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
- if (ret != 0)
- return;
-
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr,
- "Warning: TSO enabled but not supported by port %d\n",
- res->port_id);
- }
+ cmd_config_queue_tx_offloads(&ports[res->port_id]);
cmd_reconfig_device_queue(res->port_id, 1, 1);
}
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..21210aff43 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
uint64_t ol_flags = 0;
uint32_t max_pkt_len, tso_segsz = 0;
uint16_t l4_off;
+ uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
/* ensure packet is large enough to require tso */
if (!info->is_tunnel) {
@@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
/* do not recalculate udp cksum if it was 0 */
if (udp_hdr->dgram_cksum != 0) {
- if (tso_segsz)
+ if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
@@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
#endif
} else if (info->l4_proto == IPPROTO_TCP) {
tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
- if (tso_segsz)
+ if (tso_segsz &&
+ (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso)))
ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
--
2.33.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
@ 2023-10-20 3:38 ` lihuisong (C)
2023-10-27 6:15 ` fengchengwen
2023-11-03 1:31 ` Ferruh Yigit
2 siblings, 0 replies; 14+ messages in thread
From: lihuisong (C) @ 2023-10-20 3:38 UTC (permalink / raw)
To: dev; +Cc: thomas, ferruh.yigit, aman.deep.singh, yuying.zhang, zhichaox.zeng
kindly ping.
在 2023/8/2 10:55, Huisong Li 写道:
> The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
> tso_segsz is set. Then tx_prepare() may call rte_net_intel_cksum_prepare()
> to compute pseudo header checksum (because some PMDs may supports TSO).
> As a result, if the peer sends UDP packets, all packets with UDP checksum
> error are received for the PMDs only supported TSO.
>
> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
> capability. Similarly, TSO also need to do like this.
>
> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
> it better to support TSO and UFO.
>
> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> v2: add handle for tunnel TSO offload in process_inner_cksums
>
> ---
> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
> app/test-pmd/csumonly.c | 11 ++++++++--
> 2 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0d0723f659..8be593d405 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
> {
> struct cmd_tso_set_result *res = parsed_result;
> struct rte_eth_dev_info dev_info;
> + uint64_t offloads;
> int ret;
>
> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
> if (ret != 0)
> return;
>
> - if ((ports[res->port_id].tso_segsz != 0) &&
> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
> - res->port_id);
> - return;
> + if (ports[res->port_id].tso_segsz != 0) {
> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
> + fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
> + res->port_id);
> + return;
> + }
> + /* display warnings if configuration is not supported by the NIC */
> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
> + res->port_id);
> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
> + res->port_id);
> }
>
> if (ports[res->port_id].tso_segsz == 0) {
> ports[res->port_id].dev_conf.txmode.offloads &=
> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
> - printf("TSO for non-tunneled packets is disabled\n");
> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
> + printf("TSO and UFO for non-tunneled packets is disabled\n");
> } else {
> - ports[res->port_id].dev_conf.txmode.offloads |=
> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
> - printf("TSO segment size for non-tunneled packets is %d\n",
> + offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
> + offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
> + printf("segment size for non-tunneled packets is %d\n",
> ports[res->port_id].tso_segsz);
> }
> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
> -
> - /* display warnings if configuration is not supported by the NIC */
> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
> - if (ret != 0)
> - return;
> -
> - if ((ports[res->port_id].tso_segsz != 0) &&
> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
> - fprintf(stderr,
> - "Warning: TSO enabled but not supported by port %d\n",
> - res->port_id);
> - }
>
> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
> cmd_reconfig_device_queue(res->port_id, 1, 1);
> }
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index c103e54111..21210aff43 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> uint64_t ol_flags = 0;
> uint32_t max_pkt_len, tso_segsz = 0;
> uint16_t l4_off;
> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>
> /* ensure packet is large enough to require tso */
> if (!info->is_tunnel) {
> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
> /* do not recalculate udp cksum if it was 0 */
> if (udp_hdr->dgram_cksum != 0) {
> - if (tso_segsz)
> + if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> #endif
> } else if (info->l4_proto == IPPROTO_TCP) {
> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
> - if (tso_segsz)
> + if (tso_segsz &&
> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso)))
> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
2023-10-20 3:38 ` lihuisong (C)
@ 2023-10-27 6:15 ` fengchengwen
2023-11-03 1:31 ` Ferruh Yigit
2 siblings, 0 replies; 14+ messages in thread
From: fengchengwen @ 2023-10-27 6:15 UTC (permalink / raw)
To: Huisong Li, dev
Cc: thomas, ferruh.yigit, aman.deep.singh, yuying.zhang, zhichaox.zeng
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2023/8/2 10:55, Huisong Li wrote:
> The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
> tso_segsz is set. Then tx_prepare() may call rte_net_intel_cksum_prepare()
> to compute pseudo header checksum (because some PMDs may supports TSO).
> As a result, if the peer sends UDP packets, all packets with UDP checksum
> error are received for the PMDs only supported TSO.
>
> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
> capability. Similarly, TSO also need to do like this.
>
> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
> it better to support TSO and UFO.
>
> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
2023-10-20 3:38 ` lihuisong (C)
2023-10-27 6:15 ` fengchengwen
@ 2023-11-03 1:31 ` Ferruh Yigit
2023-11-03 9:09 ` lihuisong (C)
2 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2023-11-03 1:31 UTC (permalink / raw)
To: Huisong Li, dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
On 8/2/2023 3:55 AM, Huisong Li wrote:
> The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
> tso_segsz is set.
>
"The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
by checking if 'tso_segsz' is set, but missing check if UFO offload
(RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
> Then tx_prepare() may call rte_net_intel_cksum_prepare()
> to compute pseudo header checksum (because some PMDs may supports TSO).
>
Not sure what do you mean by '(because some PMDs may supports TSO)'?
Do you mean something like following:
"RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
compute pseudo header checksum."
> As a result, if the peer sends UDP packets, all packets with UDP checksum
> error are received for the PMDs only supported TSO.
>
"As a result, if device only supports TSO, but not UFO, UDP packet
checksum will be wrong."
> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
> capability. Similarly, TSO also need to do like this.
>
> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
> it better to support TSO and UFO.
>
> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> v2: add handle for tunnel TSO offload in process_inner_cksums
>
> ---
> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
> app/test-pmd/csumonly.c | 11 ++++++++--
> 2 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0d0723f659..8be593d405 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
> {
> struct cmd_tso_set_result *res = parsed_result;
> struct rte_eth_dev_info dev_info;
> + uint64_t offloads;
> int ret;
>
> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
> if (ret != 0)
> return;
>
> - if ((ports[res->port_id].tso_segsz != 0) &&
> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
> - res->port_id);
> - return;
> + if (ports[res->port_id].tso_segsz != 0) {
> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
> + fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
> + res->port_id);
> + return;
> + }
> + /* display warnings if configuration is not supported by the NIC */
> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
> + res->port_id);
> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
> + res->port_id);
>
Requesting TSO/UFO by setting 'tso_segsz', but device capability missing
is an error case, so OK to have first message.
But only supporting TSO or UFO is not an error case, not sure about
logging this. But even it is logged, I think it shouldn't be to stderr
or it should say "Warning: ", a regular logging can be done.
> }
>
> if (ports[res->port_id].tso_segsz == 0) {
> ports[res->port_id].dev_conf.txmode.offloads &=
> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
> - printf("TSO for non-tunneled packets is disabled\n");
> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
> + printf("TSO and UFO for non-tunneled packets is disabled\n");
> } else {
> - ports[res->port_id].dev_conf.txmode.offloads |=
> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
> - printf("TSO segment size for non-tunneled packets is %d\n",
> + offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
> + offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
> + printf("segment size for non-tunneled packets is %d\n",
> ports[res->port_id].tso_segsz);
> }
> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
> -
> - /* display warnings if configuration is not supported by the NIC */
> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
> - if (ret != 0)
> - return;
> -
> - if ((ports[res->port_id].tso_segsz != 0) &&
> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
> - fprintf(stderr,
> - "Warning: TSO enabled but not supported by port %d\n",
> - res->port_id);
> - }
>
Above is redundant check, and introduced with commit [1], I assume by
mistake. Since removing above check is not related to UFO, what do you
think to separate it to its own patch?
[1]
Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
> cmd_reconfig_device_queue(res->port_id, 1, 1);
> }
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index c103e54111..21210aff43 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> uint64_t ol_flags = 0;
> uint32_t max_pkt_len, tso_segsz = 0;
> uint16_t l4_off;
> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>
> /* ensure packet is large enough to require tso */
> if (!info->is_tunnel) {
> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
> /* do not recalculate udp cksum if it was 0 */
> if (udp_hdr->dgram_cksum != 0) {
> - if (tso_segsz)
> + if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
> #endif
> } else if (info->l4_proto == IPPROTO_TCP) {
> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
> - if (tso_segsz)
> + if (tso_segsz &&
> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso)))
>
Should we check 'all_tunnel_tso', and why they are checked only for TCP?
As far as I can see some tunnel TSO offloads should case setting
relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG'
flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
I assume intention is to be close to previous implementation, where only
tso_segsz checked, and cover as much as possible TSO offload requests,
but not sure if this is accurate with expected usage.
> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-03 1:31 ` Ferruh Yigit
@ 2023-11-03 9:09 ` lihuisong (C)
2023-11-03 10:42 ` Ferruh Yigit
0 siblings, 1 reply; 14+ messages in thread
From: lihuisong (C) @ 2023-11-03 9:09 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
Hi Ferruh,
Thanks for you review.
在 2023/11/3 9:31, Ferruh Yigit 写道:
> On 8/2/2023 3:55 AM, Huisong Li wrote:
>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>
>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if
>> tso_segsz is set.
>>
> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
> by checking if 'tso_segsz' is set, but missing check if UFO offload
> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
Ack
>
>
>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>> to compute pseudo header checksum (because some PMDs may supports TSO).
>>
> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>
> Do you mean something like following:
> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
> compute pseudo header checksum."
Ack
>
>
>> As a result, if the peer sends UDP packets, all packets with UDP checksum
>> error are received for the PMDs only supported TSO.
>>
> "As a result, if device only supports TSO, but not UFO, UDP packet
> checksum will be wrong."
Ack
>
>
>> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
>> capability. Similarly, TSO also need to do like this.
>>
>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
>> it better to support TSO and UFO.
>>
>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> v2: add handle for tunnel TSO offload in process_inner_cksums
>>
>> ---
>> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
>> app/test-pmd/csumonly.c | 11 ++++++++--
>> 2 files changed, 33 insertions(+), 25 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 0d0723f659..8be593d405 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>> {
>> struct cmd_tso_set_result *res = parsed_result;
>> struct rte_eth_dev_info dev_info;
>> + uint64_t offloads;
>> int ret;
>>
>> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>> if (ret != 0)
>> return;
>>
>> - if ((ports[res->port_id].tso_segsz != 0) &&
>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
>> - res->port_id);
>> - return;
>> + if (ports[res->port_id].tso_segsz != 0) {
>> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>> + fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
>> + res->port_id);
>> + return;
>> + }
>> + /* display warnings if configuration is not supported by the NIC */
>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
>> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>> + res->port_id);
>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
>> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>> + res->port_id);
>>
> Requesting TSO/UFO by setting 'tso_segsz', but device capability missing
> is an error case, so OK to have first message.
>
> But only supporting TSO or UFO is not an error case, not sure about
> logging this. But even it is logged, I think it shouldn't be to stderr
> or it should say "Warning: ", a regular logging can be done.
All right, will fix it in next version.
>
>
>> }
>>
>> if (ports[res->port_id].tso_segsz == 0) {
>> ports[res->port_id].dev_conf.txmode.offloads &=
>> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>> - printf("TSO for non-tunneled packets is disabled\n");
>> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
>> + printf("TSO and UFO for non-tunneled packets is disabled\n");
>> } else {
>> - ports[res->port_id].dev_conf.txmode.offloads |=
>> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
>> - printf("TSO segment size for non-tunneled packets is %d\n",
>> + offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>> + offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>> + printf("segment size for non-tunneled packets is %d\n",
>> ports[res->port_id].tso_segsz);
>> }
>> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
>> -
>> - /* display warnings if configuration is not supported by the NIC */
>> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>> - if (ret != 0)
>> - return;
>> -
>> - if ((ports[res->port_id].tso_segsz != 0) &&
>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>> - fprintf(stderr,
>> - "Warning: TSO enabled but not supported by port %d\n",
>> - res->port_id);
>> - }
>>
> Above is redundant check, and introduced with commit [1], I assume by
> mistake.
Yes, it is a redundant check indeed.
This check is introduced in the first patch[1]. But the patch [2] add
offload capabilities check but don't delete the old check.
[1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
engine")
[2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
> Since removing above check is not related to UFO, what do you
> think to separate it to its own patch?
ok, will separate it from this patch.
>
> [1]
> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>
>
>> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
>> cmd_reconfig_device_queue(res->port_id, 1, 1);
>> }
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index c103e54111..21210aff43 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
>> uint64_t ol_flags = 0;
>> uint32_t max_pkt_len, tso_segsz = 0;
>> uint16_t l4_off;
>> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>
>> /* ensure packet is large enough to require tso */
>> if (!info->is_tunnel) {
>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
>> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
>> /* do not recalculate udp cksum if it was 0 */
>> if (udp_hdr->dgram_cksum != 0) {
>> - if (tso_segsz)
>> + if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
>> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
>> #endif
>> } else if (info->l4_proto == IPPROTO_TCP) {
>> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
>> - if (tso_segsz)
>> + if (tso_segsz &&
>> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso)))
>>
> Should we check 'all_tunnel_tso', and why they are checked only for TCP?
Yes, this patch is just for TCP_TSO and UDP_TSO.
But here is necessary for tunnel_tso, or this doesn't set
'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>
> As far as I can see some tunnel TSO offloads should case setting
> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>
> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG'
> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
At least here didn't change the original behavior for tunnel tso.
I'm not still clear how to set these flag for tunnel tso.
But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel tso.
>
> I assume intention is to be close to previous implementation, where only
> tso_segsz checked, and cover as much as possible TSO offload requests,
> but not sure if this is accurate with expected usage.
we may need to do something for tunnel tso command as this patch did.
I will take a look at it after this patch.
>
>> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
> .
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-03 9:09 ` lihuisong (C)
@ 2023-11-03 10:42 ` Ferruh Yigit
2023-11-06 4:13 ` lihuisong (C)
0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2023-11-03 10:42 UTC (permalink / raw)
To: lihuisong (C), dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
On 11/3/2023 9:09 AM, lihuisong (C) wrote:
> Hi Ferruh,
>
> Thanks for you review.
>
>
> 在 2023/11/3 9:31, Ferruh Yigit 写道:
>> On 8/2/2023 3:55 AM, Huisong Li wrote:
>>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO,
>>> please
>>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>
>>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO
>>> only if
>>> tso_segsz is set.
>>>
>> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
>> by checking if 'tso_segsz' is set, but missing check if UFO offload
>> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
> Ack
>>
>>
>>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>>> to compute pseudo header checksum (because some PMDs may supports TSO).
>>>
>> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>>
>> Do you mean something like following:
>> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
>> compute pseudo header checksum."
> Ack
>>
>>
>>> As a result, if the peer sends UDP packets, all packets with UDP
>>> checksum
>>> error are received for the PMDs only supported TSO.
>>>
>> "As a result, if device only supports TSO, but not UFO, UDP packet
>> checksum will be wrong."
> Ack
>>
>>
>>> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
>>> capability. Similarly, TSO also need to do like this.
>>>
>>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
>>> it better to support TSO and UFO.
>>>
>>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>> v2: add handle for tunnel TSO offload in process_inner_cksums
>>>
>>> ---
>>> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
>>> app/test-pmd/csumonly.c | 11 ++++++++--
>>> 2 files changed, 33 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 0d0723f659..8be593d405 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>>> {
>>> struct cmd_tso_set_result *res = parsed_result;
>>> struct rte_eth_dev_info dev_info;
>>> + uint64_t offloads;
>>> int ret;
>>> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>>> if (ret != 0)
>>> return;
>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
>>> - res->port_id);
>>> - return;
>>> + if (ports[res->port_id].tso_segsz != 0) {
>>> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>>> + fprintf(stderr, "Error: both TSO and UFO are not
>>> supported by port %d\n",
>>> + res->port_id);
>>> + return;
>>> + }
>>> + /* display warnings if configuration is not supported by the
>>> NIC */
>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO)
>>> == 0)
>>> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>>> + res->port_id);
>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO)
>>> == 0)
>>> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>>> + res->port_id);
>>>
>> Requesting TSO/UFO by setting 'tso_segsz', but device capability missing
>> is an error case, so OK to have first message.
>>
>> But only supporting TSO or UFO is not an error case, not sure about
>> logging this. But even it is logged, I think it shouldn't be to stderr
>> or it should say "Warning: ", a regular logging can be done.
> All right, will fix it in next version.
>>
>>
>>> }
>>> if (ports[res->port_id].tso_segsz == 0) {
>>> ports[res->port_id].dev_conf.txmode.offloads &=
>>> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>> - printf("TSO for non-tunneled packets is disabled\n");
>>> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
>>> + printf("TSO and UFO for non-tunneled packets is disabled\n");
>>> } else {
>>> - ports[res->port_id].dev_conf.txmode.offloads |=
>>> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>> - printf("TSO segment size for non-tunneled packets is %d\n",
>>> + offloads = (dev_info.tx_offload_capa &
>>> RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>>> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>>> + offloads |= (dev_info.tx_offload_capa &
>>> RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>>> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>>> + printf("segment size for non-tunneled packets is %d\n",
>>> ports[res->port_id].tso_segsz);
>>> }
>>> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>> -
>>> - /* display warnings if configuration is not supported by the NIC */
>>> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>> - if (ret != 0)
>>> - return;
>>> -
>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>> - fprintf(stderr,
>>> - "Warning: TSO enabled but not supported by port %d\n",
>>> - res->port_id);
>>> - }
>>>
>> Above is redundant check, and introduced with commit [1], I assume by
>> mistake.
> Yes, it is a redundant check indeed.
> This check is introduced in the first patch[1]. But the patch [2] add
> offload capabilities check but don't delete the old check.
>
>
> [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
> engine")
> [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>> Since removing above check is not related to UFO, what do you
>> think to separate it to its own patch?
> ok, will separate it from this patch.
>>
>> [1]
>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>
>>
>>
>>> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>> cmd_reconfig_device_queue(res->port_id, 1, 1);
>>> }
>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>> index c103e54111..21210aff43 100644
>>> --- a/app/test-pmd/csumonly.c
>>> +++ b/app/test-pmd/csumonly.c
>>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct
>>> testpmd_offload_info *info,
>>> uint64_t ol_flags = 0;
>>> uint32_t max_pkt_len, tso_segsz = 0;
>>> uint16_t l4_off;
>>> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>>> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>>> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>>> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>>> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>> /* ensure packet is large enough to require tso */
>>> if (!info->is_tunnel) {
>>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct
>>> testpmd_offload_info *info,
>>> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr +
>>> info->l3_len);
>>> /* do not recalculate udp cksum if it was 0 */
>>> if (udp_hdr->dgram_cksum != 0) {
>>> - if (tso_segsz)
>>> + if (tso_segsz && (tx_offloads &
>>> RTE_ETH_TX_OFFLOAD_UDP_TSO))
>>> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct
>>> testpmd_offload_info *info,
>>> #endif
>>> } else if (info->l4_proto == IPPROTO_TCP) {
>>> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr +
>>> info->l3_len);
>>> - if (tso_segsz)
>>> + if (tso_segsz &&
>>> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>> all_tunnel_tso)))
>>>
>> Should we check 'all_tunnel_tso', and why they are checked only for TCP?
> Yes, this patch is just for TCP_TSO and UDP_TSO.
> But here is necessary for tunnel_tso, or this doesn't set
> 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>
Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but
'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the
'RTE_MBUF_F_TX_TCP_SEG' flag?
I am not really clear how to handle tunnel TSO offloads, but considering
previous implementation was only relying on 'tso_segsz', continue with
all TSO offload will be similar to previous implementation, so OK to
have it.
And with same logic, should we add 'all_tunnel_tso' check to the UDP case?
And agree that setting other tunnel related mbuf flags is out of scope
for this patch, but probably that part is missing in this code, and only
a few drivers support these flags anyway.
>>
>> As far as I can see some tunnel TSO offloads should case setting
>> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
>> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>>
>> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
>> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG'
>> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
> At least here didn't change the original behavior for tunnel tso.
> I'm not still clear how to set these flag for tunnel tso.
> But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel tso.
>>
>> I assume intention is to be close to previous implementation, where only
>> tso_segsz checked, and cover as much as possible TSO offload requests,
>> but not sure if this is accurate with expected usage.
> we may need to do something for tunnel tso command as this patch did.
> I will take a look at it after this patch.
>>
>>> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>> .
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-03 10:42 ` Ferruh Yigit
@ 2023-11-06 4:13 ` lihuisong (C)
2023-11-06 10:09 ` Ferruh Yigit
0 siblings, 1 reply; 14+ messages in thread
From: lihuisong (C) @ 2023-11-06 4:13 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
在 2023/11/3 18:42, Ferruh Yigit 写道:
> On 11/3/2023 9:09 AM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> Thanks for you review.
>>
>>
>> 在 2023/11/3 9:31, Ferruh Yigit 写道:
>>> On 8/2/2023 3:55 AM, Huisong Li wrote:
>>>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO,
>>>> please
>>>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>
>>>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO
>>>> only if
>>>> tso_segsz is set.
>>>>
>>> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
>>> by checking if 'tso_segsz' is set, but missing check if UFO offload
>>> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
>> Ack
>>>
>>>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>>>> to compute pseudo header checksum (because some PMDs may supports TSO).
>>>>
>>> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>>>
>>> Do you mean something like following:
>>> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
>>> compute pseudo header checksum."
>> Ack
>>>
>>>> As a result, if the peer sends UDP packets, all packets with UDP
>>>> checksum
>>>> error are received for the PMDs only supported TSO.
>>>>
>>> "As a result, if device only supports TSO, but not UFO, UDP packet
>>> checksum will be wrong."
>> Ack
>>>
>>>> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
>>>> capability. Similarly, TSO also need to do like this.
>>>>
>>>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
>>>> it better to support TSO and UFO.
>>>>
>>>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>> v2: add handle for tunnel TSO offload in process_inner_cksums
>>>>
>>>> ---
>>>> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++--------------------
>>>> app/test-pmd/csumonly.c | 11 ++++++++--
>>>> 2 files changed, 33 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 0d0723f659..8be593d405 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>>>> {
>>>> struct cmd_tso_set_result *res = parsed_result;
>>>> struct rte_eth_dev_info dev_info;
>>>> + uint64_t offloads;
>>>> int ret;
>>>> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>>>> if (ret != 0)
>>>> return;
>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>>> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
>>>> - res->port_id);
>>>> - return;
>>>> + if (ports[res->port_id].tso_segsz != 0) {
>>>> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>>>> + fprintf(stderr, "Error: both TSO and UFO are not
>>>> supported by port %d\n",
>>>> + res->port_id);
>>>> + return;
>>>> + }
>>>> + /* display warnings if configuration is not supported by the
>>>> NIC */
>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO)
>>>> == 0)
>>>> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>>>> + res->port_id);
>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO)
>>>> == 0)
>>>> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>>>> + res->port_id);
>>>>
>>> Requesting TSO/UFO by setting 'tso_segsz', but device capability missing
>>> is an error case, so OK to have first message.
>>>
>>> But only supporting TSO or UFO is not an error case, not sure about
>>> logging this. But even it is logged, I think it shouldn't be to stderr
>>> or it should say "Warning: ", a regular logging can be done.
>> All right, will fix it in next version.
>>>
>>>> }
>>>> if (ports[res->port_id].tso_segsz == 0) {
>>>> ports[res->port_id].dev_conf.txmode.offloads &=
>>>> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>> - printf("TSO for non-tunneled packets is disabled\n");
>>>> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
>>>> + printf("TSO and UFO for non-tunneled packets is disabled\n");
>>>> } else {
>>>> - ports[res->port_id].dev_conf.txmode.offloads |=
>>>> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>> - printf("TSO segment size for non-tunneled packets is %d\n",
>>>> + offloads = (dev_info.tx_offload_capa &
>>>> RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>>>> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>>>> + offloads |= (dev_info.tx_offload_capa &
>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>>>> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>>>> + printf("segment size for non-tunneled packets is %d\n",
>>>> ports[res->port_id].tso_segsz);
>>>> }
>>>> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>> -
>>>> - /* display warnings if configuration is not supported by the NIC */
>>>> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>>> - if (ret != 0)
>>>> - return;
>>>> -
>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>>> - fprintf(stderr,
>>>> - "Warning: TSO enabled but not supported by port %d\n",
>>>> - res->port_id);
>>>> - }
>>>>
>>> Above is redundant check, and introduced with commit [1], I assume by
>>> mistake.
>> Yes, it is a redundant check indeed.
>> This check is introduced in the first patch[1]. But the patch [2] add
>> offload capabilities check but don't delete the old check.
>>
>>
>> [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
>> engine")
>> [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>> Since removing above check is not related to UFO, what do you
>>> think to separate it to its own patch?
>> ok, will separate it from this patch.
>>> [1]
>>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>>
>>>> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>> cmd_reconfig_device_queue(res->port_id, 1, 1);
>>>> }
>>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>>> index c103e54111..21210aff43 100644
>>>> --- a/app/test-pmd/csumonly.c
>>>> +++ b/app/test-pmd/csumonly.c
>>>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>> uint64_t ol_flags = 0;
>>>> uint32_t max_pkt_len, tso_segsz = 0;
>>>> uint16_t l4_off;
>>>> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>>>> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>>> /* ensure packet is large enough to require tso */
>>>> if (!info->is_tunnel) {
>>>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr +
>>>> info->l3_len);
>>>> /* do not recalculate udp cksum if it was 0 */
>>>> if (udp_hdr->dgram_cksum != 0) {
>>>> - if (tso_segsz)
>>>> + if (tso_segsz && (tx_offloads &
>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO))
>>>> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>> #endif
>>>> } else if (info->l4_proto == IPPROTO_TCP) {
>>>> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr +
>>>> info->l3_len);
>>>> - if (tso_segsz)
>>>> + if (tso_segsz &&
>>>> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>> all_tunnel_tso)))
>>>>
>>> Should we check 'all_tunnel_tso', and why they are checked only for TCP?
>> Yes, this patch is just for TCP_TSO and UDP_TSO.
>> But here is necessary for tunnel_tso, or this doesn't set
>> 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>>
> Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but
> 'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the
> 'RTE_MBUF_F_TX_TCP_SEG' flag?
Yes, RTE_MBUF_F_TX_TCP_SEG flag still should be set for tunnel tso.
Driver compute pseudo header checksum and fill hw descriptor based on
this flag.
>
> I am not really clear how to handle tunnel TSO offloads, but considering
> previous implementation was only relying on 'tso_segsz', continue with
> all TSO offload will be similar to previous implementation, so OK to
> have it.
Yes
>
> And with same logic, should we add 'all_tunnel_tso' check to the UDP case?
I didn't see some offloads about UDP_TSO for tunnel packet.
And testpmd just support a command (please see
cmd_tunnel_tso_set_parsed) to set these tunnel tso offloads this patch
mentioned.
>
>
> And agree that setting other tunnel related mbuf flags is out of scope
> for this patch, but probably that part is missing in this code, and only
What specific thing is missing in this code?
> a few drivers support these flags anyway.
>
>>> As far as I can see some tunnel TSO offloads should case setting
>>> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
>>> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>>>
>>> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
>>> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG'
>>> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
>> At least here didn't change the original behavior for tunnel tso.
>> I'm not still clear how to set these flag for tunnel tso.
>> But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel tso.
>>> I assume intention is to be close to previous implementation, where only
>>> tso_segsz checked, and cover as much as possible TSO offload requests,
>>> but not sure if this is accurate with expected usage.
>> we may need to do something for tunnel tso command as this patch did.
>> I will take a look at it after this patch.
>>>> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>>> .
> .
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-06 4:13 ` lihuisong (C)
@ 2023-11-06 10:09 ` Ferruh Yigit
2023-11-06 12:29 ` lihuisong (C)
0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2023-11-06 10:09 UTC (permalink / raw)
To: lihuisong (C), dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
On 11/6/2023 4:13 AM, lihuisong (C) wrote:
>
> 在 2023/11/3 18:42, Ferruh Yigit 写道:
>> On 11/3/2023 9:09 AM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> Thanks for you review.
>>>
>>>
>>> 在 2023/11/3 9:31, Ferruh Yigit 写道:
>>>> On 8/2/2023 3:55 AM, Huisong Li wrote:
>>>>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO,
>>>>> please
>>>>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum
>>>>> engine")
>>>>>
>>>>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO
>>>>> only if
>>>>> tso_segsz is set.
>>>>>
>>>> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
>>>> by checking if 'tso_segsz' is set, but missing check if UFO offload
>>>> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
>>> Ack
>>>>
>>>>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>>>>> to compute pseudo header checksum (because some PMDs may supports
>>>>> TSO).
>>>>>
>>>> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>>>>
>>>> Do you mean something like following:
>>>> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
>>>> compute pseudo header checksum."
>>> Ack
>>>>
>>>>> As a result, if the peer sends UDP packets, all packets with UDP
>>>>> checksum
>>>>> error are received for the PMDs only supported TSO.
>>>>>
>>>> "As a result, if device only supports TSO, but not UFO, UDP packet
>>>> checksum will be wrong."
>>> Ack
>>>>
>>>>> So enabling UFO also depends on if driver has
>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO
>>>>> capability. Similarly, TSO also need to do like this.
>>>>>
>>>>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to
>>>>> make
>>>>> it better to support TSO and UFO.
>>>>>
>>>>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> ---
>>>>> v2: add handle for tunnel TSO offload in process_inner_cksums
>>>>>
>>>>> ---
>>>>> app/test-pmd/cmdline.c | 47
>>>>> +++++++++++++++++++++--------------------
>>>>> app/test-pmd/csumonly.c | 11 ++++++++--
>>>>> 2 files changed, 33 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>>> index 0d0723f659..8be593d405 100644
>>>>> --- a/app/test-pmd/cmdline.c
>>>>> +++ b/app/test-pmd/cmdline.c
>>>>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>> {
>>>>> struct cmd_tso_set_result *res = parsed_result;
>>>>> struct rte_eth_dev_info dev_info;
>>>>> + uint64_t offloads;
>>>>> int ret;
>>>>> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>>>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>> if (ret != 0)
>>>>> return;
>>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ==
>>>>> 0) {
>>>>> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
>>>>> - res->port_id);
>>>>> - return;
>>>>> + if (ports[res->port_id].tso_segsz != 0) {
>>>>> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>>>>> + fprintf(stderr, "Error: both TSO and UFO are not
>>>>> supported by port %d\n",
>>>>> + res->port_id);
>>>>> + return;
>>>>> + }
>>>>> + /* display warnings if configuration is not supported by the
>>>>> NIC */
>>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO)
>>>>> == 0)
>>>>> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>>>>> + res->port_id);
>>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO)
>>>>> == 0)
>>>>> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>>>>> + res->port_id);
>>>>>
>>>> Requesting TSO/UFO by setting 'tso_segsz', but device capability
>>>> missing
>>>> is an error case, so OK to have first message.
>>>>
>>>> But only supporting TSO or UFO is not an error case, not sure about
>>>> logging this. But even it is logged, I think it shouldn't be to stderr
>>>> or it should say "Warning: ", a regular logging can be done.
>>> All right, will fix it in next version.
>>>>
>>>>> }
>>>>> if (ports[res->port_id].tso_segsz == 0) {
>>>>> ports[res->port_id].dev_conf.txmode.offloads &=
>>>>> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>>> - printf("TSO for non-tunneled packets is disabled\n");
>>>>> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO);
>>>>> + printf("TSO and UFO for non-tunneled packets is disabled\n");
>>>>> } else {
>>>>> - ports[res->port_id].dev_conf.txmode.offloads |=
>>>>> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>>> - printf("TSO segment size for non-tunneled packets is %d\n",
>>>>> + offloads = (dev_info.tx_offload_capa &
>>>>> RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>>>>> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>>>>> + offloads |= (dev_info.tx_offload_capa &
>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>>>>> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>>>>> + printf("segment size for non-tunneled packets is %d\n",
>>>>> ports[res->port_id].tso_segsz);
>>>>> }
>>>>> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>>> -
>>>>> - /* display warnings if configuration is not supported by the
>>>>> NIC */
>>>>> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>>>> - if (ret != 0)
>>>>> - return;
>>>>> -
>>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ==
>>>>> 0) {
>>>>> - fprintf(stderr,
>>>>> - "Warning: TSO enabled but not supported by port %d\n",
>>>>> - res->port_id);
>>>>> - }
>>>>>
>>>> Above is redundant check, and introduced with commit [1], I assume by
>>>> mistake.
>>> Yes, it is a redundant check indeed.
>>> This check is introduced in the first patch[1]. But the patch [2] add
>>> offload capabilities check but don't delete the old check.
>>>
>>>
>>> [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
>>> engine")
>>> [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities
>>> check")
>>>> Since removing above check is not related to UFO, what do you
>>>> think to separate it to its own patch?
>>> ok, will separate it from this patch.
>>>> [1]
>>>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>>>
>>>>> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>>> cmd_reconfig_device_queue(res->port_id, 1, 1);
>>>>> }
>>>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>>>> index c103e54111..21210aff43 100644
>>>>> --- a/app/test-pmd/csumonly.c
>>>>> +++ b/app/test-pmd/csumonly.c
>>>>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>> testpmd_offload_info *info,
>>>>> uint64_t ol_flags = 0;
>>>>> uint32_t max_pkt_len, tso_segsz = 0;
>>>>> uint16_t l4_off;
>>>>> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>>>> /* ensure packet is large enough to require tso */
>>>>> if (!info->is_tunnel) {
>>>>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>> testpmd_offload_info *info,
>>>>> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr +
>>>>> info->l3_len);
>>>>> /* do not recalculate udp cksum if it was 0 */
>>>>> if (udp_hdr->dgram_cksum != 0) {
>>>>> - if (tso_segsz)
>>>>> + if (tso_segsz && (tx_offloads &
>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO))
>>>>> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>>>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>>>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>> testpmd_offload_info *info,
>>>>> #endif
>>>>> } else if (info->l4_proto == IPPROTO_TCP) {
>>>>> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr +
>>>>> info->l3_len);
>>>>> - if (tso_segsz)
>>>>> + if (tso_segsz &&
>>>>> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>> all_tunnel_tso)))
>>>>>
>>>> Should we check 'all_tunnel_tso', and why they are checked only for
>>>> TCP?
>>> Yes, this patch is just for TCP_TSO and UDP_TSO.
>>> But here is necessary for tunnel_tso, or this doesn't set
>>> 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>>>
>> Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but
>> 'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the
>> 'RTE_MBUF_F_TX_TCP_SEG' flag?
> Yes, RTE_MBUF_F_TX_TCP_SEG flag still should be set for tunnel tso.
> Driver compute pseudo header checksum and fill hw descriptor based on
> this flag.
>>
>> I am not really clear how to handle tunnel TSO offloads, but considering
>> previous implementation was only relying on 'tso_segsz', continue with
>> all TSO offload will be similar to previous implementation, so OK to
>> have it.
> Yes
>>
>> And with same logic, should we add 'all_tunnel_tso' check to the UDP
>> case?
> I didn't see some offloads about UDP_TSO for tunnel packet.
> And testpmd just support a command (please see
> cmd_tunnel_tso_set_parsed) to set these tunnel tso offloads this patch
> mentioned.
>>
>>
>> And agree that setting other tunnel related mbuf flags is out of scope
>> for this patch, but probably that part is missing in this code, and only
> What specific thing is missing in this code?
>
I don't mean this patch, but existing code. It doesn't set
'RTE_MBUF_F_TX_TUNNEL_UDP' or 'RTE_MBUF_F_TX_TUNNEL_IP' mbuf flags when
relevant TSO offload requested.
>> a few drivers support these flags anyway.
>>
>>>> As far as I can see some tunnel TSO offloads should case setting
>>>> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
>>>> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>>>>
>>>> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
>>>> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set
>>>> 'RTE_MBUF_F_TX_TCP_SEG'
>>>> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
>>> At least here didn't change the original behavior for tunnel tso.
>>> I'm not still clear how to set these flag for tunnel tso.
>>> But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel
>>> tso.
>>>> I assume intention is to be close to previous implementation, where
>>>> only
>>>> tso_segsz checked, and cover as much as possible TSO offload requests,
>>>> but not sure if this is accurate with expected usage.
>>> we may need to do something for tunnel tso command as this patch did.
>>> I will take a look at it after this patch.
>>>>> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>>>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>>>> .
>> .
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-06 10:09 ` Ferruh Yigit
@ 2023-11-06 12:29 ` lihuisong (C)
0 siblings, 0 replies; 14+ messages in thread
From: lihuisong (C) @ 2023-11-06 12:29 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
在 2023/11/6 18:09, Ferruh Yigit 写道:
> On 11/6/2023 4:13 AM, lihuisong (C) wrote:
>> 在 2023/11/3 18:42, Ferruh Yigit 写道:
>>> On 11/3/2023 9:09 AM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> Thanks for you review.
>>>>
>>>>
>>>> 在 2023/11/3 9:31, Ferruh Yigit 写道:
>>>>> On 8/2/2023 3:55 AM, Huisong Li wrote:
>>>>>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO,
>>>>>> please
>>>>>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum
>>>>>> engine")
>>>>>>
>>>>>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO
>>>>>> only if
>>>>>> tso_segsz is set.
>>>>>>
>>>>> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
>>>>> by checking if 'tso_segsz' is set, but missing check if UFO offload
>>>>> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
>>>> Ack
>>>>>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>>>>>> to compute pseudo header checksum (because some PMDs may supports
>>>>>> TSO).
>>>>>>
>>>>> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>>>>>
>>>>> Do you mean something like following:
>>>>> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
>>>>> compute pseudo header checksum."
>>>> Ack
>>>>>> As a result, if the peer sends UDP packets, all packets with UDP
>>>>>> checksum
>>>>>> error are received for the PMDs only supported TSO.
>>>>>>
>>>>> "As a result, if device only supports TSO, but not UFO, UDP packet
>>>>> checksum will be wrong."
>>>> Ack
>>>>>> So enabling UFO also depends on if driver has
>>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO
>>>>>> capability. Similarly, TSO also need to do like this.
>>>>>>
>>>>>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to
>>>>>> make
>>>>>> it better to support TSO and UFO.
>>>>>>
>>>>>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> ---
>>>>>> v2: add handle for tunnel TSO offload in process_inner_cksums
>>>>>>
>>>>>> ---
>>>>>> app/test-pmd/cmdline.c | 47
>>>>>> +++++++++++++++++++++--------------------
>>>>>> app/test-pmd/csumonly.c | 11 ++++++++--
>>>>>> 2 files changed, 33 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>>>> index 0d0723f659..8be593d405 100644
>>>>>> --- a/app/test-pmd/cmdline.c
>>>>>> +++ b/app/test-pmd/cmdline.c
>>>>>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>>> {
>>>>>> struct cmd_tso_set_result *res = parsed_result;
>>>>>> struct rte_eth_dev_info dev_info;
>>>>>> + uint64_t offloads;
>>>>>> int ret;
>>>>>> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>>>>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>>> if (ret != 0)
>>>>>> return;
>>>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ==
>>>>>> 0) {
>>>>>> - fprintf(stderr, "Error: TSO is not supported by port %d\n",
>>>>>> - res->port_id);
>>>>>> - return;
>>>>>> + if (ports[res->port_id].tso_segsz != 0) {
>>>>>> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>>>>>> + fprintf(stderr, "Error: both TSO and UFO are not
>>>>>> supported by port %d\n",
>>>>>> + res->port_id);
>>>>>> + return;
>>>>>> + }
>>>>>> + /* display warnings if configuration is not supported by the
>>>>>> NIC */
>>>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO)
>>>>>> == 0)
>>>>>> + fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>>>>>> + res->port_id);
>>>>>> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO)
>>>>>> == 0)
>>>>>> + fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>>>>>> + res->port_id);
>>>>>>
>>>>> Requesting TSO/UFO by setting 'tso_segsz', but device capability
>>>>> missing
>>>>> is an error case, so OK to have first message.
>>>>>
>>>>> But only supporting TSO or UFO is not an error case, not sure about
>>>>> logging this. But even it is logged, I think it shouldn't be to stderr
>>>>> or it should say "Warning: ", a regular logging can be done.
>>>> All right, will fix it in next version.
>>>>>> }
>>>>>> if (ports[res->port_id].tso_segsz == 0) {
>>>>>> ports[res->port_id].dev_conf.txmode.offloads &=
>>>>>> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>>>> - printf("TSO for non-tunneled packets is disabled\n");
>>>>>> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO);
>>>>>> + printf("TSO and UFO for non-tunneled packets is disabled\n");
>>>>>> } else {
>>>>>> - ports[res->port_id].dev_conf.txmode.offloads |=
>>>>>> - RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>>>> - printf("TSO segment size for non-tunneled packets is %d\n",
>>>>>> + offloads = (dev_info.tx_offload_capa &
>>>>>> RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>>>>>> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>>>>>> + offloads |= (dev_info.tx_offload_capa &
>>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>>>>>> + ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>>>>>> + printf("segment size for non-tunneled packets is %d\n",
>>>>>> ports[res->port_id].tso_segsz);
>>>>>> }
>>>>>> - cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>>>> -
>>>>>> - /* display warnings if configuration is not supported by the
>>>>>> NIC */
>>>>>> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>>>>> - if (ret != 0)
>>>>>> - return;
>>>>>> -
>>>>>> - if ((ports[res->port_id].tso_segsz != 0) &&
>>>>>> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ==
>>>>>> 0) {
>>>>>> - fprintf(stderr,
>>>>>> - "Warning: TSO enabled but not supported by port %d\n",
>>>>>> - res->port_id);
>>>>>> - }
>>>>>>
>>>>> Above is redundant check, and introduced with commit [1], I assume by
>>>>> mistake.
>>>> Yes, it is a redundant check indeed.
>>>> This check is introduced in the first patch[1]. But the patch [2] add
>>>> offload capabilities check but don't delete the old check.
>>>>
>>>>
>>>> [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
>>>> engine")
>>>> [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities
>>>> check")
>>>>> Since removing above check is not related to UFO, what do you
>>>>> think to separate it to its own patch?
>>>> ok, will separate it from this patch.
>>>>> [1]
>>>>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>>>>
>>>>>> + cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>>>> cmd_reconfig_device_queue(res->port_id, 1, 1);
>>>>>> }
>>>>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>>>>> index c103e54111..21210aff43 100644
>>>>>> --- a/app/test-pmd/csumonly.c
>>>>>> +++ b/app/test-pmd/csumonly.c
>>>>>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>>> testpmd_offload_info *info,
>>>>>> uint64_t ol_flags = 0;
>>>>>> uint32_t max_pkt_len, tso_segsz = 0;
>>>>>> uint16_t l4_off;
>>>>>> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>>>>>> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>>>>> /* ensure packet is large enough to require tso */
>>>>>> if (!info->is_tunnel) {
>>>>>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>>> testpmd_offload_info *info,
>>>>>> udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr +
>>>>>> info->l3_len);
>>>>>> /* do not recalculate udp cksum if it was 0 */
>>>>>> if (udp_hdr->dgram_cksum != 0) {
>>>>>> - if (tso_segsz)
>>>>>> + if (tso_segsz && (tx_offloads &
>>>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO))
>>>>>> ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>>>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>>>>> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>>>>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct
>>>>>> testpmd_offload_info *info,
>>>>>> #endif
>>>>>> } else if (info->l4_proto == IPPROTO_TCP) {
>>>>>> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr +
>>>>>> info->l3_len);
>>>>>> - if (tso_segsz)
>>>>>> + if (tso_segsz &&
>>>>>> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>>>> all_tunnel_tso)))
>>>>>>
>>>>> Should we check 'all_tunnel_tso', and why they are checked only for
>>>>> TCP?
>>>> Yes, this patch is just for TCP_TSO and UDP_TSO.
>>>> But here is necessary for tunnel_tso, or this doesn't set
>>>> 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>>>>
>>> Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but
>>> 'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the
>>> 'RTE_MBUF_F_TX_TCP_SEG' flag?
>> Yes, RTE_MBUF_F_TX_TCP_SEG flag still should be set for tunnel tso.
>> Driver compute pseudo header checksum and fill hw descriptor based on
>> this flag.
>>> I am not really clear how to handle tunnel TSO offloads, but considering
>>> previous implementation was only relying on 'tso_segsz', continue with
>>> all TSO offload will be similar to previous implementation, so OK to
>>> have it.
>> Yes
>>> And with same logic, should we add 'all_tunnel_tso' check to the UDP
>>> case?
>> I didn't see some offloads about UDP_TSO for tunnel packet.
>> And testpmd just support a command (please see
>> cmd_tunnel_tso_set_parsed) to set these tunnel tso offloads this patch
>> mentioned.
>>>
>>> And agree that setting other tunnel related mbuf flags is out of scope
>>> for this patch, but probably that part is missing in this code, and only
>> What specific thing is missing in this code?
>>
> I don't mean this patch, but existing code. It doesn't set
> 'RTE_MBUF_F_TX_TUNNEL_UDP' or 'RTE_MBUF_F_TX_TUNNEL_IP' mbuf flags when
> relevant TSO offload requested.
get it.
I will check "cmd_tunnel_tso_set_parsed" after this patch and see if we
need to do something for tunnel tso in testpmd.
Will send v3 according to your advice ASAP.
>
>
>
>>> a few drivers support these flags anyway.
>>>
>>>>> As far as I can see some tunnel TSO offloads should case setting
>>>>> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
>>>>> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>>>>>
>>>>> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only
>>>>> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set
>>>>> 'RTE_MBUF_F_TX_TCP_SEG'
>>>>> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
>>>> At least here didn't change the original behavior for tunnel tso.
>>>> I'm not still clear how to set these flag for tunnel tso.
>>>> But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel
>>>> tso.
>>>>> I assume intention is to be close to previous implementation, where
>>>>> only
>>>>> tso_segsz checked, and cover as much as possible TSO offload requests,
>>>>> but not sure if this is accurate with expected usage.
>>>> we may need to do something for tunnel tso command as this patch did.
>>>> I will take a look at it after this patch.
>>>>>> ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>>>>>> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>>>>> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>>>>> .
>>> .
> .
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 0/2] app/testpmd: fix UDP cksum error for UFO enable
2023-07-28 2:13 [PATCH] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
@ 2023-11-07 4:11 ` Huisong Li
2023-11-07 4:11 ` [PATCH v3 1/2] app/testpmd: remove useless code for TSO setting command Huisong Li
` (2 more replies)
1 sibling, 3 replies; 14+ messages in thread
From: Huisong Li @ 2023-11-07 4:11 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, aman.deep.singh, yuying.zhang,
zhichaox.zeng, lihuisong
1. remove useless code.
2. fix UDP cksum error for UFO enable.
For detail message, please see the commit log.
---
v3:
- separate the useless code from patch v2.
- fix some comments and use 'printf' to replace 'fprintf'.
v2: add handle for tunnel TSO offload in process_inner_cksums
Huisong Li (2):
app/testpmd: remove useless code for TSO setting command
app/testpmd: fix UDP cksum error for UFO enable
app/test-pmd/cmdline.c | 44 ++++++++++++++++++++---------------------
app/test-pmd/csumonly.c | 11 +++++++++--
2 files changed, 30 insertions(+), 25 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] app/testpmd: remove useless code for TSO setting command
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
@ 2023-11-07 4:11 ` Huisong Li
2023-11-07 4:11 ` [PATCH v3 2/2] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
2023-11-07 9:54 ` [PATCH v3 0/2] " Ferruh Yigit
2 siblings, 0 replies; 14+ messages in thread
From: Huisong Li @ 2023-11-07 4:11 UTC (permalink / raw)
To: dev, Aman Singh, Yuying Zhang, Wenzhuo Lu, Shahaf Shuler
Cc: thomas, ferruh.yigit, zhichaox.zeng, lihuisong
Testpmd has added the check of TSO offload capability of port, please see
the commit 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
So the code following the check code memtioned above to display warning
when port doesn't support TSO offload doesn't access to forever.
Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
app/test-pmd/cmdline.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 912bf3355c..1c57e07d41 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4967,19 +4967,6 @@ cmd_tso_set_parsed(void *parsed_result,
ports[res->port_id].tso_segsz);
}
cmd_config_queue_tx_offloads(&ports[res->port_id]);
-
- /* display warnings if configuration is not supported by the NIC */
- ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
- if (ret != 0)
- return;
-
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr,
- "Warning: TSO enabled but not supported by port %d\n",
- res->port_id);
- }
-
cmd_reconfig_device_queue(res->port_id, 1, 1);
}
--
2.33.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
2023-11-07 4:11 ` [PATCH v3 1/2] app/testpmd: remove useless code for TSO setting command Huisong Li
@ 2023-11-07 4:11 ` Huisong Li
2023-11-07 9:54 ` [PATCH v3 0/2] " Ferruh Yigit
2 siblings, 0 replies; 14+ messages in thread
From: Huisong Li @ 2023-11-07 4:11 UTC (permalink / raw)
To: dev, Aman Singh, Yuying Zhang, Zhichao Zeng
Cc: thomas, ferruh.yigit, lihuisong
The command "tso set <tso_segsz> <port_id>" is used to enable UFO, please
see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
by checking if 'tso_segsz' is set, but missing check if UFO offload
(RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device. The RTE_MBUF_F_TX_UDP_SEG
flag causes driver that supports TSO to compute pseudo header checksum.
As a result, if device only supports TSO, but not UFO, UDP packet checksum
will be wrong.
So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
capability. Similarly, TSO also need to do like this.
Note: all offloads about tunnel TSO are added in process_inner_cksums() in
case of the impact on tunnel TSO.
In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
it better to support TSO and UFO.
Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
app/test-pmd/cmdline.c | 31 +++++++++++++++++++++----------
app/test-pmd/csumonly.c | 11 +++++++++--
2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1c57e07d41..05078f8377 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4933,6 +4933,7 @@ cmd_tso_set_parsed(void *parsed_result,
{
struct cmd_tso_set_result *res = parsed_result;
struct rte_eth_dev_info dev_info;
+ uint64_t offloads;
int ret;
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
@@ -4949,21 +4950,31 @@ cmd_tso_set_parsed(void *parsed_result,
if (ret != 0)
return;
- if ((ports[res->port_id].tso_segsz != 0) &&
- (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
- fprintf(stderr, "Error: TSO is not supported by port %d\n",
- res->port_id);
- return;
+ if (ports[res->port_id].tso_segsz != 0) {
+ if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
+ RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
+ fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n",
+ res->port_id);
+ return;
+ }
+ /* display warnings if configuration is not supported by the NIC */
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0)
+ printf("Warning: port %d doesn't support TSO\n", res->port_id);
+ if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0)
+ printf("Warning: port %d doesn't support UFO\n", res->port_id);
}
if (ports[res->port_id].tso_segsz == 0) {
ports[res->port_id].dev_conf.txmode.offloads &=
- ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO for non-tunneled packets is disabled\n");
+ ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
+ printf("TSO and UFO for non-tunneled packets is disabled\n");
} else {
- ports[res->port_id].dev_conf.txmode.offloads |=
- RTE_ETH_TX_OFFLOAD_TCP_TSO;
- printf("TSO segment size for non-tunneled packets is %d\n",
+ offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
+ offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
+ RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
+ ports[res->port_id].dev_conf.txmode.offloads |= offloads;
+ printf("segment size for non-tunneled packets is %d\n",
ports[res->port_id].tso_segsz);
}
cmd_config_queue_tx_offloads(&ports[res->port_id]);
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..21210aff43 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
uint64_t ol_flags = 0;
uint32_t max_pkt_len, tso_segsz = 0;
uint16_t l4_off;
+ uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
+ RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
/* ensure packet is large enough to require tso */
if (!info->is_tunnel) {
@@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
/* do not recalculate udp cksum if it was 0 */
if (udp_hdr->dgram_cksum != 0) {
- if (tso_segsz)
+ if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO))
ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
@@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
#endif
} else if (info->l4_proto == IPPROTO_TCP) {
tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
- if (tso_segsz)
+ if (tso_segsz &&
+ (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso)))
ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
--
2.33.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] app/testpmd: fix UDP cksum error for UFO enable
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
2023-11-07 4:11 ` [PATCH v3 1/2] app/testpmd: remove useless code for TSO setting command Huisong Li
2023-11-07 4:11 ` [PATCH v3 2/2] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
@ 2023-11-07 9:54 ` Ferruh Yigit
2 siblings, 0 replies; 14+ messages in thread
From: Ferruh Yigit @ 2023-11-07 9:54 UTC (permalink / raw)
To: Huisong Li, dev; +Cc: thomas, aman.deep.singh, yuying.zhang, zhichaox.zeng
On 11/7/2023 4:11 AM, Huisong Li wrote:
> 1. remove useless code.
> 2. fix UDP cksum error for UFO enable.
> For detail message, please see the commit log.
>
> ---
> v3:
> - separate the useless code from patch v2.
> - fix some comments and use 'printf' to replace 'fprintf'.
>
> v2: add handle for tunnel TSO offload in process_inner_cksums
>
> Huisong Li (2):
> app/testpmd: remove useless code for TSO setting command
> app/testpmd: fix UDP cksum error for UFO enable
>
For series,
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-11-07 9:55 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-28 2:13 [PATCH] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
2023-08-02 2:55 ` [PATCH v2] " Huisong Li
2023-10-20 3:38 ` lihuisong (C)
2023-10-27 6:15 ` fengchengwen
2023-11-03 1:31 ` Ferruh Yigit
2023-11-03 9:09 ` lihuisong (C)
2023-11-03 10:42 ` Ferruh Yigit
2023-11-06 4:13 ` lihuisong (C)
2023-11-06 10:09 ` Ferruh Yigit
2023-11-06 12:29 ` lihuisong (C)
2023-11-07 4:11 ` [PATCH v3 0/2] " Huisong Li
2023-11-07 4:11 ` [PATCH v3 1/2] app/testpmd: remove useless code for TSO setting command Huisong Li
2023-11-07 4:11 ` [PATCH v3 2/2] app/testpmd: fix UDP cksum error for UFO enable Huisong Li
2023-11-07 9:54 ` [PATCH v3 0/2] " Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).