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

["lihuisong (C)" <lihuisong@huawei.com>]

Hi Ferruh,

Thanks for you review so fast.

在 2023/11/11 11:30, Ferruh Yigit 写道:
> 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?
ok, will fixed in next version.
>
>>   	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);
>>   
> .