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: sta...@dpdk.org
>>>
>>> Signed-off-by: Jiawen Wu <jiawe...@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.
