Re: [PATCH v1 2/3] app/testpmd: add the explicit check for tunnel TSO offload

[Ferruh Yigit <ferruh.yigit@amd.com>]

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: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@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);
>