[dpdk-dev] [RFC PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[dpdk-dev] [RFC PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Ivan Malov]

Make sure that outer L2 and L3 header length fields are
equal to zero for non-tunnel packets in order to ensure
consistent and predictable behaviour in network drivers.
Explain this expectation in comments to help developers.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---

Notes:
    At the time of writing a couple of network drivers rely on
    the statement (i40e, ice) whilst more drivers have runtime
    conditional checks to guard all references to these fields.
    This patch is likely to relieve datapath checks in drivers.

 lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f7886dc..9d316ef 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -696,7 +696,12 @@ struct rte_mbuf {
 			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
 			/**< TCP TSO segment size */
 
-			/* fields for TX offloading of tunnels */
+			/*
+			 * Fields for Tx offloading of tunnels.
+			 * These fields must be equal to zero in the case
+			 * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0,
+			 * i.e. for all non-tunnel packets.
+			 */
 			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
 			/**< Outer L3 (IP) Hdr Length. */
 			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
@@ -2370,6 +2375,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
 			!(ol_flags & PKT_TX_OUTER_IPV4))
 		return -EINVAL;
 
+	/* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */
+	if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 &&
+	    m->outer_l2_len + m->outer_l3_len != 0)
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
1.8.3.1

[dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Ivan Malov]

Make sure that outer L2 and L3 header length fields are
equal to zero for non-tunnel packets in order to ensure
consistent and predictable behaviour in network drivers.
Explain this expectation in comments to help developers.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---

Notes:
    At the time of writing a couple of network drivers rely on
    the statement (i40e, ice) whilst more drivers have runtime
    conditional checks to guard all references to these fields.
    This patch is likely to relieve datapath checks in drivers.

 lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 0d9fef0..cb8b34e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -702,7 +702,12 @@ struct rte_mbuf {
 			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
 			/**< TCP TSO segment size */
 
-			/* fields for TX offloading of tunnels */
+			/*
+			 * Fields for Tx offloading of tunnels.
+			 * These fields must be equal to zero in the case
+			 * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0,
+			 * i.e. for all non-tunnel packets.
+			 */
 			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
 			/**< Outer L3 (IP) Hdr Length. */
 			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
@@ -2376,6 +2381,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
 			!(ol_flags & PKT_TX_OUTER_IPV4))
 		return -EINVAL;
 
+	/* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */
+	if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 &&
+	    m->outer_l2_len + m->outer_l3_len != 0)
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Ananyev, Konstantin]

Hi Ivan,

> 
> Make sure that outer L2 and L3 header length fields are
> equal to zero for non-tunnel packets in order to ensure
> consistent and predictable behaviour in network drivers.
> Explain this expectation in comments to help developers.
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---

Not sure it is a good idea:
1) it is a change in public API behavior (requirements).
2) why these 2 particular tx_offload fields only?
If we'll follow that logic we should enforce same rule for other
tx_offload fileds (tso, l4_len, l3_len, etc.)

Personally I don't think there will be much gain from it.
Might be better and easier just to fix offending drivers that make wrong assumptions.

If we'll still decide to go that way, then I think at least it needs
to be explained in RN, and probably deprecation process has to be followed here.
 
Konstantin

> 
> Notes:
>     At the time of writing a couple of network drivers rely on
>     the statement (i40e, ice) whilst more drivers have runtime
>     conditional checks to guard all references to these fields.
>     This patch is likely to relieve datapath checks in drivers.
> 
>  lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 0d9fef0..cb8b34e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -702,7 +702,12 @@ struct rte_mbuf {
>  			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
>  			/**< TCP TSO segment size */
> 
> -			/* fields for TX offloading of tunnels */
> +			/*
> +			 * Fields for Tx offloading of tunnels.
> +			 * These fields must be equal to zero in the case
> +			 * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0,
> +			 * i.e. for all non-tunnel packets.
> +			 */
>  			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
>  			/**< Outer L3 (IP) Hdr Length. */
>  			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
> @@ -2376,6 +2381,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>  			!(ol_flags & PKT_TX_OUTER_IPV4))
>  		return -EINVAL;
> 
> +	/* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */
> +	if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 &&
> +	    m->outer_l2_len + m->outer_l3_len != 0)
> +		return -EINVAL;
> +
>  	return 0;
>  }
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Andrew Rybchenko]

Hi Konstantin,

On 6/21/19 2:10 PM, Ananyev, Konstantin wrote:
> Hi Ivan,
>> Make sure that outer L2 and L3 header length fields are
>> equal to zero for non-tunnel packets in order to ensure
>> consistent and predictable behaviour in network drivers.
>> Explain this expectation in comments to help developers.
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
> Not sure it is a good idea:
> 1) it is a change in public API behavior (requirements).

I would say that it is a clarification. Yes, in terms of 
rte_validate_tx_offload()
behaviour is it is a change. The area looks grey and we just want to make
it either black or white. What is the alternative? Say that outer_l2_len and
outer_l3_len content is undefined if packet is not tunnelled and drivers
must check (ol_flags & PKT_TX_TUNNEL_MASK) != 0 before usage these fields?

bnxt, fm10k, i40e, ixgbe (depends on PKT_TX_OUTER_IP_CKSUM in fact, but
not PKT_TX_TUNNEL_MASK) and ice use these fields w/o tunnel checks (if
I read code correctly).

enic, mlx4, mlx5, qede and sfc use them in the case of tunnel packet only.

I.e. 5 vs 5.

> 2) why these 2 particular tx_offload fields only?
> If we'll follow that logic we should enforce same rule for other
> tx_offload fileds (tso, l4_len, l3_len, etc.)

Because it is about tunnel packets and outer_l2_len and outer_l3_len
should be either undefined or 0 for non-tunnel packets.

> Personally I don't think there will be much gain from it.
> Might be better and easier just to fix offending drivers that make wrong assumptions.

We would prefer to define as the patch suggests since it allows
to avoid conditions. Other option is to add a comment saying that
content of these fields is undefined for non-tunnel packets.
Of course, the patch makes it required to care about outer_l2/3_len
when mbuf is reused and Tx offloads are requested. So, may be
from application point of view it is better to have it undefined for
non-tunnel packets.

> If we'll still decide to go that way, then I think at least it needs
> to be explained in RN, and probably deprecation process has to be followed here.

Yes, I agree and would like to understand which way is right
(just highlight in release notes or deprecation process).

BTW, may I ask you to take a look at two more small patches:
[1] https://patches.dpdk.org/patch/53691/
[2] https://patches.dpdk.org/patch/53857/

Many thanks,
Andrew.

(As Keith said some time ago it looks like almost nobody look at RFC
patches. Sad. The main goal of RFC patches is get feedback earlier.
RFC for this one was in April and we could start deprecation process
in previous release cycle if it is required. Luckily it is not critical
in this case.)

> Konstantin
>
>> Notes:
>>      At the time of writing a couple of network drivers rely on
>>      the statement (i40e, ice) whilst more drivers have runtime
>>      conditional checks to guard all references to these fields.
>>      This patch is likely to relieve datapath checks in drivers.
>>
>>   lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 0d9fef0..cb8b34e 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -702,7 +702,12 @@ struct rte_mbuf {
>>   			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
>>   			/**< TCP TSO segment size */
>>
>> -			/* fields for TX offloading of tunnels */
>> +			/*
>> +			 * Fields for Tx offloading of tunnels.
>> +			 * These fields must be equal to zero in the case
>> +			 * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0,
>> +			 * i.e. for all non-tunnel packets.
>> +			 */
>>   			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
>>   			/**< Outer L3 (IP) Hdr Length. */
>>   			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
>> @@ -2376,6 +2381,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>>   			!(ol_flags & PKT_TX_OUTER_IPV4))
>>   		return -EINVAL;
>>
>> +	/* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */
>> +	if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 &&
>> +	    m->outer_l2_len + m->outer_l3_len != 0)
>> +		return -EINVAL;
>> +
>>   	return 0;
>>   }
>>
>> --
>> 1.8.3.1

Re: [dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Ananyev, Konstantin]

Hi Andrew,

> Hi Konstantin,
> 
> On 6/21/19 2:10 PM, Ananyev, Konstantin wrote:
> Hi Ivan,
> 
> Make sure that outer L2 and L3 header length fields are
> equal to zero for non-tunnel packets in order to ensure
> consistent and predictable behaviour in network drivers.
> Explain this expectation in comments to help developers.
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> 
>> Not sure it is a good idea:
>> 1) it is a change in public API behavior (requirements).
> 
> I would say that it is a clarification. Yes, in terms of rte_validate_tx_offload()
> behaviour is it is a change. The area looks grey and we just want to make
> it either black or white. What is the alternative? Say that outer_l2_len and
> outer_l3_len content is undefined if packet is not tunnelled and drivers
> must check (ol_flags & PKT_TX_TUNNEL_MASK) != 0 before usage these fields?

Yes, that was my thought.
As I understand, that what is implied right now.
Otherwise any app that setups tx_offload fieds for rte_eth_tx_burst()
need to be changed?

> 
> bnxt, fm10k, i40e, ixgbe (depends on PKT_TX_OUTER_IP_CKSUM in fact, but
> not PKT_TX_TUNNEL_MASK) and ice use these fields w/o tunnel checks (if
> I read code correctly).
> 
> enic, mlx4, mlx5, qede and sfc use them in the case of tunnel packet only.
> 
> I.e. 5 vs 5.
> 
> 
>> 2) why these 2 particular tx_offload fields only?
>> If we'll follow that logic we should enforce same rule for other
>> tx_offload fileds (tso, l4_len, l3_len, etc.)
> 
> Because it is about tunnel packets and outer_l2_len and outer_l3_len
> should be either undefined or 0 for non-tunnel packets.

I understand that, but I think rules for setting/treating tx_offload fields
should be the same for all fields.
We either allow any tx_offload field to be undefined when related
bit(s) in ol_flags are not set, or we need to force people to setup
whole 64-bit tx_offload value if any of related TX flags are set.

> 
> 
>> Personally I don't think there will be much gain from it.
>> Might be better and easier just to fix offending drivers that make wrong assumptions.
> 
> We would prefer to define as the patch suggests since it allows
> to avoid conditions.

It does, and it might simplify things for PMDs...
But as I said above, it would need changes in the apps that
do use tx_offload fileds for TX, right?

> Other option is to add a comment saying that
> content of these fields is undefined for non-tunnel packets.
> Of course, the patch makes it required to care about outer_l2/3_len
> when mbuf is reused and Tx offloads are requested. So, may be
> from application point of view it is better to have it undefined for
> non-tunnel packets.
> 
> 
> If we'll still decide to go that way, then I think at least it needs
> to be explained in RN, and probably deprecation process has to be followed here.
> 
> Yes, I agree and would like to understand which way is right
> (just highlight in release notes or deprecation process).

From my understanding: if changes inside app code might be necessary,
then we do need a deprecation note. 

> 
> BTW, may I ask you to take a look at two more small patches:
> [1] https://patches.dpdk.org/patch/53691/
> [2] https://patches.dpdk.org/patch/53857/

Will do

> 
> Many thanks,
> Andrew.
> 
> (As Keith said some time ago it looks like almost nobody look at RFC
> patches. Sad. The main goal of RFC patches is get feedback earlier.
> RFC for this one was in April and we could start deprecation process
> in previous release cycle if it is required. Luckily it is not critical
> in this case.)
> 
> 
> Konstantin
> 
> 
> Notes:
>     At the time of writing a couple of network drivers rely on
>     the statement (i40e, ice) whilst more drivers have runtime
>     conditional checks to guard all references to these fields.
>     This patch is likely to relieve datapath checks in drivers.
> 
>  lib/librte_mbuf/rte_mbuf.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 0d9fef0..cb8b34e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -702,7 +702,12 @@ struct rte_mbuf {
>  			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
>  			/**< TCP TSO segment size */
> 
> -			/* fields for TX offloading of tunnels */
> +			/*
> +			 * Fields for Tx offloading of tunnels.
> +			 * These fields must be equal to zero in the case
> +			 * when (ol_flags & PKT_TX_TUNNEL_MASK) == 0,
> +			 * i.e. for all non-tunnel packets.
> +			 */
>  			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
>  			/**< Outer L3 (IP) Hdr Length. */
>  			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
> @@ -2376,6 +2381,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>  			!(ol_flags & PKT_TX_OUTER_IPV4))
>  		return -EINVAL;
> 
> +	/* Outer L2/L3 offsets must be equal to zero for non-tunnel packets. */
> +	if ((ol_flags & PKT_TX_TUNNEL_MASK) == 0 &&
> +	    m->outer_l2_len + m->outer_l3_len != 0)
> +		return -EINVAL;
> +
>  	return 0;
>  }
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH] mbuf: outer offsets must be zero for non-tunnel packets

[Andrew Rybchenko]

On 6/24/19 3:59 PM, Ananyev, Konstantin wrote:
> Hi Andrew,
>
>> Hi Konstantin,
>>
>> On 6/21/19 2:10 PM, Ananyev, Konstantin wrote:
>> Hi Ivan,
>>
>> Make sure that outer L2 and L3 header length fields are
>> equal to zero for non-tunnel packets in order to ensure
>> consistent and predictable behaviour in network drivers.
>> Explain this expectation in comments to help developers.
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>
>>> Not sure it is a good idea:
>>> 1) it is a change in public API behavior (requirements).
>> I would say that it is a clarification. Yes, in terms of rte_validate_tx_offload()
>> behaviour is it is a change. The area looks grey and we just want to make
>> it either black or white. What is the alternative? Say that outer_l2_len and
>> outer_l3_len content is undefined if packet is not tunnelled and drivers
>> must check (ol_flags & PKT_TX_TUNNEL_MASK) != 0 before usage these fields?
> Yes, that was my thought.
> As I understand, that what is implied right now.
> Otherwise any app that setups tx_offload fieds for rte_eth_tx_burst()
> need to be changed?
>
>> bnxt, fm10k, i40e, ixgbe (depends on PKT_TX_OUTER_IP_CKSUM in fact, but
>> not PKT_TX_TUNNEL_MASK) and ice use these fields w/o tunnel checks (if
>> I read code correctly).
>>
>> enic, mlx4, mlx5, qede and sfc use them in the case of tunnel packet only.
>>
>> I.e. 5 vs 5.
>>
>>
>>> 2) why these 2 particular tx_offload fields only?
>>> If we'll follow that logic we should enforce same rule for other
>>> tx_offload fileds (tso, l4_len, l3_len, etc.)
>> Because it is about tunnel packets and outer_l2_len and outer_l3_len
>> should be either undefined or 0 for non-tunnel packets.
> I understand that, but I think rules for setting/treating tx_offload fields
> should be the same for all fields.
> We either allow any tx_offload field to be undefined when related
> bit(s) in ol_flags are not set, or we need to force people to setup
> whole 64-bit tx_offload value if any of related TX flags are set.

Yes, I agree.

>>> Personally I don't think there will be much gain from it.
>>> Might be better and easier just to fix offending drivers that make wrong assumptions.
>> We would prefer to define as the patch suggests since it allows
>> to avoid conditions.
> It does, and it might simplify things for PMDs...
> But as I said above, it would need changes in the apps that
> do use tx_offload fileds for TX, right?

Yes, it sounds bad. OK, we have discussed it and Ivan will send v2
which simply clarify comments.

It looks like there is no simple conditions when we should require
(in validate function) non-zero outer_l2/l3_len.

Thanks a lot,
Andrew.

[dpdk-dev] [PATCH v2] mbuf: outer offsets are undefined for non-tunnel packets

[Ivan Malov]

The default policy for offload-specific fields is that
they are undefined unless the corresponding offloads are
requested in mbuf ol_flags. This is also the case for outer
L2 and L3 length fields which must not be assumed to contain
zeros for non-tunnel packets. The patch clarifies this behaviour
in the comments. PMDs which mistakenly assume these fields
to be zero for non-tunnel packets are expected to comply
with the clarified comment and have dedicated fixes.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
---
 lib/librte_mbuf/rte_mbuf.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 0d9fef0..26a0b14 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -702,7 +702,18 @@ struct rte_mbuf {
 			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
 			/**< TCP TSO segment size */
 
-			/* fields for TX offloading of tunnels */
+			/*
+			 * Fields for Tx offloading of tunnels.
+			 * These are undefined for packets which don't request
+			 * any tunnel offloads (outer IP or UDP checksum,
+			 * tunnel TSO).
+			 *
+			 * PMDs are advised not to use these fields
+			 * unconditionally when calculating offsets.
+			 *
+			 * Applications are expected to set appropriate tunnel
+			 * offload flags when they fill in these fields.
+			 */
 			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
 			/**< Outer L3 (IP) Hdr Length. */
 			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] mbuf: outer offsets are undefined for non-tunnel packets

[Ananyev, Konstantin]

> The default policy for offload-specific fields is that
> they are undefined unless the corresponding offloads are
> requested in mbuf ol_flags. This is also the case for outer
> L2 and L3 length fields which must not be assumed to contain
> zeros for non-tunnel packets. The patch clarifies this behaviour
> in the comments. PMDs which mistakenly assume these fields
> to be zero for non-tunnel packets are expected to comply
> with the clarified comment and have dedicated fixes.
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 0d9fef0..26a0b14 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -702,7 +702,18 @@ struct rte_mbuf {
>  			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
>  			/**< TCP TSO segment size */
> 
> -			/* fields for TX offloading of tunnels */
> +			/*
> +			 * Fields for Tx offloading of tunnels.
> +			 * These are undefined for packets which don't request
> +			 * any tunnel offloads (outer IP or UDP checksum,
> +			 * tunnel TSO).
> +			 *
> +			 * PMDs are advised not to use these fields
> +			 * unconditionally when calculating offsets.

I'd use something stronger then 'are advised not to'.
Might be something like: 'should not use'.
BTW, from the previous discussion, right now we do
have some misbehaving PMDs that need to be fixed, correct?
Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com> 

> +			 *
> +			 * Applications are expected to set appropriate tunnel
> +			 * offload flags when they fill in these fields.
> +			 */
>  			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
>  			/**< Outer L3 (IP) Hdr Length. */
>  			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
> --
> 1.8.3.1

[dpdk-dev] [PATCH v3] mbuf: outer offsets are undefined for non-tunnel packets

[Ivan Malov]

The default policy for offload-specific fields is that
they are undefined unless the corresponding offloads are
requested in mbuf ol_flags. This is also the case for outer
L2 and L3 length fields which must not be assumed to contain
zeros for non-tunnel packets. The patch clarifies this behaviour
in the comments and also adds appropriate checks to the PMDs which
do not check any tunnel-related offloads before using the said fields.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/bnxt/bnxt_txr.c    |  6 ++++--
 drivers/net/fm10k/fm10k_rxtx.c |  5 +++--
 drivers/net/i40e/i40e_rxtx.c   | 12 +++---------
 drivers/net/iavf/iavf_rxtx.c   |  3 ---
 drivers/net/ice/ice_rxtx.c     | 12 +++---------
 lib/librte_mbuf/rte_mbuf.h     | 13 ++++++++++++-
 6 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
index 124186e..c71e6f1 100644
--- a/drivers/net/bnxt/bnxt_txr.c
+++ b/drivers/net/bnxt/bnxt_txr.c
@@ -235,8 +235,10 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
 			txbd1->lflags |= TX_BD_LONG_LFLAGS_LSO |
 					 TX_BD_LONG_LFLAGS_T_IPID;
 			hdr_size = tx_pkt->l2_len + tx_pkt->l3_len +
-					tx_pkt->l4_len + tx_pkt->outer_l2_len +
-					tx_pkt->outer_l3_len;
+					tx_pkt->l4_len;
+			hdr_size += (tx_pkt->ol_flags & PKT_TX_TUNNEL_MASK) ?
+				    tx_pkt->outer_l2_len +
+				    tx_pkt->outer_l3_len : 0;
 			/* The hdr_size is multiple of 16bit units not 8bit.
 			 * Hence divide by 2.
 			 */
diff --git a/drivers/net/fm10k/fm10k_rxtx.c b/drivers/net/fm10k/fm10k_rxtx.c
index 2f4dad0..5c31121 100644
--- a/drivers/net/fm10k/fm10k_rxtx.c
+++ b/drivers/net/fm10k/fm10k_rxtx.c
@@ -619,8 +619,9 @@ static inline void tx_xmit_pkt(struct fm10k_tx_queue *q, struct rte_mbuf *mb)
 			rte_cpu_to_le_16(rte_pktmbuf_data_len(mb));
 
 	if (mb->ol_flags & PKT_TX_TCP_SEG) {
-		hdrlen = mb->outer_l2_len + mb->outer_l3_len + mb->l2_len +
-			mb->l3_len + mb->l4_len;
+		hdrlen = mb->l2_len + mb->l3_len + mb->l4_len;
+		hdrlen += (mb->ol_flags & PKT_TX_TUNNEL_MASK) ?
+			  mb->outer_l2_len + mb->outer_l3_len : 0;
 		if (q->hw_ring[q->next_free].flags & FM10K_TXD_FLAG_FTAG)
 			hdrlen += sizeof(struct fm10k_ftag);
 
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 85c44f5..34aa6c8 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -981,15 +981,9 @@
 		return ctx_desc;
 	}
 
-	/**
-	 * in case of non tunneling packet, the outer_l2_len and
-	 * outer_l3_len must be 0.
-	 */
-	hdr_len = tx_offload.outer_l2_len +
-		tx_offload.outer_l3_len +
-		tx_offload.l2_len +
-		tx_offload.l3_len +
-		tx_offload.l4_len;
+	hdr_len = tx_offload.l2_len + tx_offload.l3_len + tx_offload.l4_len;
+	hdr_len += (mbuf->ol_flags & PKT_TX_TUNNEL_MASK) ?
+		   tx_offload.outer_l2_len + tx_offload.outer_l3_len : 0;
 
 	cd_cmd = I40E_TX_CTX_DESC_TSO;
 	cd_tso_len = mbuf->pkt_len - hdr_len;
diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index 0be3ede..2d37c17 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -1449,9 +1449,6 @@
 		return ctx_desc;
 	}
 
-	/* in case of non tunneling packet, the outer_l2_len and
-	 * outer_l3_len must be 0.
-	 */
 	hdr_len = tx_offload.l2_len +
 		  tx_offload.l3_len +
 		  tx_offload.l4_len;
diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0fb3e7f..035ed84 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -1940,15 +1940,9 @@
 		return ctx_desc;
 	}
 
-	/**
-	 * in case of non tunneling packet, the outer_l2_len and
-	 * outer_l3_len must be 0.
-	 */
-	hdr_len = tx_offload.outer_l2_len +
-		  tx_offload.outer_l3_len +
-		  tx_offload.l2_len +
-		  tx_offload.l3_len +
-		  tx_offload.l4_len;
+	hdr_len = tx_offload.l2_len + tx_offload.l3_len + tx_offload.l4_len;
+	hdr_len += (mbuf->ol_flags & PKT_TX_TUNNEL_MASK) ?
+		   tx_offload.outer_l2_len + tx_offload.outer_l3_len : 0;
 
 	cd_cmd = ICE_TX_CTX_DESC_TSO;
 	cd_tso_len = mbuf->pkt_len - hdr_len;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 0d9fef0..ddb562f 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -702,7 +702,18 @@ struct rte_mbuf {
 			uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
 			/**< TCP TSO segment size */
 
-			/* fields for TX offloading of tunnels */
+			/*
+			 * Fields for Tx offloading of tunnels.
+			 * These are undefined for packets which don't request
+			 * any tunnel offloads (outer IP or UDP checksum,
+			 * tunnel TSO).
+			 *
+			 * PMDs should not use these fields unconditionally
+			 * when calculating offsets.
+			 *
+			 * Applications are expected to set appropriate tunnel
+			 * offload flags when they fill in these fields.
+			 */
 			uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
 			/**< Outer L3 (IP) Hdr Length. */
 			uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] mbuf: outer offsets are undefined for non-tunnel packets

[Olivier Matz]

On Fri, Jun 28, 2019 at 12:06:18AM +0300, Ivan Malov wrote:
> The default policy for offload-specific fields is that
> they are undefined unless the corresponding offloads are
> requested in mbuf ol_flags. This is also the case for outer
> L2 and L3 length fields which must not be assumed to contain
> zeros for non-tunnel packets. The patch clarifies this behaviour
> in the comments and also adds appropriate checks to the PMDs which
> do not check any tunnel-related offloads before using the said fields.
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v3] mbuf: outer offsets are undefined for non-tunnel packets

[Thomas Monjalon]

01/07/2019 15:10, Olivier Matz:
> On Fri, Jun 28, 2019 at 12:06:18AM +0300, Ivan Malov wrote:
> > The default policy for offload-specific fields is that
> > they are undefined unless the corresponding offloads are
> > requested in mbuf ol_flags. This is also the case for outer
> > L2 and L3 length fields which must not be assumed to contain
> > zeros for non-tunnel packets. The patch clarifies this behaviour
> > in the comments and also adds appropriate checks to the PMDs which
> > do not check any tunnel-related offloads before using the said fields.
> > 
> > Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks