Re: [PATCH 3/8] net/txgbe: fix packet type to parse from offload flags

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

On 2/1/2023 3:14 AM, Jiawen Wu wrote:
> On Friday, January 27, 2023 11:37 PM, Ferruh Yigit wrote:
>> On 1/18/2023 6:00 AM, Jiawen Wu wrote:
>>> In some external applications, developers may fill in wrong
>>> packet_type in rte_mbuf for transmission. It will result in Tx ring
>>> hang when Tx checksum offload is on. So change it to parse from ol_flags.
>>>
>>
>> Can you please give more information on what packet_type value is causing
>> problem in Tx path?
>>
>>> Fixes: ca46fcd753b1 ("net/txgbe: support Tx with hardware offload")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>> ---
>>>  drivers/net/txgbe/txgbe_rxtx.c | 19 ++++++++++++++-----
>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/txgbe/txgbe_rxtx.c
>>> b/drivers/net/txgbe/txgbe_rxtx.c index ae70ca3beb..e91aaf60ef 100644
>>> --- a/drivers/net/txgbe/txgbe_rxtx.c
>>> +++ b/drivers/net/txgbe/txgbe_rxtx.c
>>> @@ -516,20 +516,21 @@ tx_desc_ol_flags_to_cmdtype(uint64_t ol_flags)
>>>  	return cmdtype;
>>>  }
>>>
>>> -static inline uint8_t
>>> -tx_desc_ol_flags_to_ptid(uint64_t oflags, uint32_t ptype)
>>> +static inline uint32_t
>>> +tx_desc_ol_flags_to_ptype(uint64_t oflags)
>>>  {
>>> +	uint32_t ptype;
>>>  	bool tun;
>>>
>>> -	if (ptype)
>>> -		return txgbe_encode_ptype(ptype);
>>> -
>>>  	/* Only support flags in TXGBE_TX_OFFLOAD_MASK */
>>>  	tun = !!(oflags & RTE_MBUF_F_TX_TUNNEL_MASK);
>>>
>>>  	/* L2 level */
>>>  	ptype = RTE_PTYPE_L2_ETHER;
>>>  	if (oflags & RTE_MBUF_F_TX_VLAN)
>>> +		ptype |= (tun ? RTE_PTYPE_INNER_L2_ETHER_VLAN :
>>> +RTE_PTYPE_L2_ETHER_VLAN);
>>> +
>>> +	if (oflags & RTE_MBUF_F_TX_QINQ) //tun + qinq is not supported
>>
>> checkpatch is complaining about c99 comment syntax ('//').
>>
>>>  		ptype |= RTE_PTYPE_L2_ETHER_VLAN;
>>>
>>>  	/* L3 level */
>>> @@ -587,6 +588,14 @@ tx_desc_ol_flags_to_ptid(uint64_t oflags,
>>> uint32_t ptype)
>>>  		break;
>>>  	}
>>>
>>> +	return ptype;
>>> +}
>>> +
>>> +static inline uint8_t
>>> +tx_desc_ol_flags_to_ptid(uint64_t oflags, uint32_t ptype) {
>>> +	ptype = tx_desc_ol_flags_to_ptype(oflags);
>>> +
>>
>> This function get 'ptype' as parameter and immediately overwrites is with
>> calculated value, what is the point of getting 'ptype' as argument.
>>
>>>  	return txgbe_encode_ptype(ptype);
>>>  }
>>>
>>
>> Overall why 'ptype' is calculated for Tx path, I see this value is used to see if it is
>> tunneled packet or not, is there any other usage of ptype in this path? If not why
>> parse all packet types?
>>
> 
> If Tx checksum offload or TSO is on, a context descriptor is needed for our hardware, which contains the length of each packet layer and the packet type.
> If the packet type and length do not strictly match, it will cause Tx ring hang. It's not just for tunnel packets. But most cases are caused by developers encap/decap at the application layer.
> However, we can flexibly set the packet type. For example, if there is no inner checksum for a VXLAN packet, we can only regard it as a UDP packet.
> 
> 

Thanks for clarification, can you please expand commit log with above
information in next version.