[3/8] net/txgbe: fix packet type to parse from offload flags
Checks
Commit Message
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.
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(-)
Comments
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?
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.
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.
@@ -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
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);
+
return txgbe_encode_ptype(ptype);
}