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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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