On 11/10/2023 8:19 AM, Huisong Li wrote:
> If port don't support TSO of tunnel packets, tell user in advance and
> no need to change other configuration of this port in case of fault spread.
>
> In addition, if some tunnel offloads don't support, which is not an error
> case, the log about this shouldn't be to stderr.
>
Overall patch looks good to me, please check a minor comment below.
> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Huisong Li <lihuis...@huawei.com>
> ---
> app/test-pmd/cmdline.c | 51 +++++++++++++++++++-----------------------
> 1 file changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 25462bdbfc..d3243d016b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -5039,28 +5039,22 @@ static void
> check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
> {
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
> - fprintf(stderr,
> - "Warning: VXLAN TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: VXLAN TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
> - fprintf(stderr,
> - "Warning: GRE TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: GRE TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
> - fprintf(stderr,
> - "Warning: IPIP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: IPIP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
> - fprintf(stderr,
> - "Warning: GENEVE TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: GENEVE TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
> - fprintf(stderr,
> - "Warning: IP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: IP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
> - fprintf(stderr,
> - "Warning: UDP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> + printf("Warning: UDP TUNNEL TSO not supported therefore not
> enabled for port %d\n",
> port_id);
> }
>
> @@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
> {
> struct cmd_tunnel_tso_set_result *res = parsed_result;
> struct rte_eth_dev_info dev_info;
> + 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;
> int ret;
>
> if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> @@ -5087,26 +5087,21 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
> if (ret != 0)
> return;
>
> - check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> + if (ports[res->port_id].tunnel_tso_segsz != 0) {
> + if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) {
> + fprintf(stderr, "Error: port=%u don't support tunnel
> TSO offloads.\n",
> + res->port_id);
> + return;
> + }
> + check_tunnel_tso_nic_support(res->port_id,
> dev_info.tx_offload_capa);
> + }
> +
Instead of having a separate if block, else leg of below if block does
same check [1], what do you think to move above block to there?
> if (ports[res->port_id].tunnel_tso_segsz == 0) {
> - ports[res->port_id].dev_conf.txmode.offloads &=
> - ~(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);
> + ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
> printf("TSO for tunneled packets is disabled\n");
> } else {
> - uint64_t tso_offloads = (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);
> -
[1] ---> here has same logical condition
> ports[res->port_id].dev_conf.txmode.offloads |=
> - (tso_offloads & dev_info.tx_offload_capa);
> + (all_tunnel_tso & dev_info.tx_offload_capa);
> printf("TSO segment size for tunneled packets is %d\n",
> ports[res->port_id].tunnel_tso_segsz);
>
