Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable

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

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;
> .