Re: [dpdk-dev] [PATCH v2] mbuf: new flag when Vlan is stripped

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

Hi Olivier,

> 
> The behavior of PKT_RX_VLAN_PKT was not very well defined, resulting in
> PMDs not advertising the same flags in similar conditions.
> 
> Following discussion in [1], introduce 2 new flags PKT_RX_VLAN_STRIPPED
> and PKT_RX_QINQ_STRIPPED that are better defined:
> 
>   PKT_RX_VLAN_STRIPPED: a vlan has been stripped by the hardware and its
>   tci is saved in mbuf->vlan_tci. This can only happen if vlan stripping
>   is enabled in the RX configuration of the PMD.
> 
> For now, the old flag PKT_RX_VLAN_PKT is kept but marked as deprecated.
> It should be removed from applications and PMDs in a future revision.

I am not sure it has to be deprecated & removed.
ixgbe (and igb as I can read the specs) devices can provide information is that
a vlan packet or not even when vlan stripping is disabled. 
Right now ixgbe PMD do carry thins information to the user,
and I suppose igb could be improved to carry it too.
So obviously we need a way to pass that information to the upper layer.
I remember it was a discussion about introducing new packet_type
instead of ol_flag value PKT_RX_VLAN_PKT.
But right now it is not there, and again I don't know how easy it would be to replace
one with another without performance considering that packet_type is not supported
now by ixgbe vRX.
If we would be able to replace it, then yes we can deprecate and drop the   PKT_RX_VLAN_PKT.
But till then, I think we'd better keep it.

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> index e97ea82..d895bf1 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> @@ -140,10 +140,9 @@ ixgbe_rxq_rearm(struct ixgbe_rx_queue *rxq)
>   */
>  #ifdef RTE_IXGBE_RX_OLFLAGS_ENABLE
> 
> -#define VTAG_SHIFT     (3)
> -
>  static inline void
> -desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
> +desc_to_olflags_v(__m128i descs[4], uint8_t vlan_flags,
> +	struct rte_mbuf **rx_pkts)
>  {
>  	__m128i ptype0, ptype1, vtag0, vtag1;
>  	union {
> @@ -151,12 +150,6 @@ desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
>  		uint64_t dword;
>  	} vol;
> 
> -	/* pkt type + vlan olflags mask */
> -	const __m128i pkttype_msk = _mm_set_epi16(
> -			0x0000, 0x0000, 0x0000, 0x0000,
> -			PKT_RX_VLAN_PKT, PKT_RX_VLAN_PKT,
> -			PKT_RX_VLAN_PKT, PKT_RX_VLAN_PKT);
> -
>  	/* mask everything except rss type */
>  	const __m128i rsstype_msk = _mm_set_epi16(
>  			0x0000, 0x0000, 0x0000, 0x0000,
> @@ -168,6 +161,19 @@ desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
>  			PKT_RX_RSS_HASH, 0, PKT_RX_RSS_HASH, 0,
>  			PKT_RX_RSS_HASH, PKT_RX_RSS_HASH, PKT_RX_RSS_HASH, 0);
> 
> +	/* mask everything except vlan present bit */
> +	const __m128i vlan_msk = _mm_set_epi16(
> +			0x0000, 0x0000,
> +			0x0000, 0x0000,
> +			IXGBE_RXD_STAT_VP, IXGBE_RXD_STAT_VP,
> +			IXGBE_RXD_STAT_VP, IXGBE_RXD_STAT_VP);
> +	/* map vlan present (0x8) to ol_flags */
> +	const __m128i vlan_map = _mm_set_epi8(
> +		0, 0, 0, 0,
> +		0, 0, 0, vlan_flags,
> +		0, 0, 0, 0,
> +		0, 0, 0, 0);
> +
>  	ptype0 = _mm_unpacklo_epi16(descs[0], descs[1]);
>  	ptype1 = _mm_unpacklo_epi16(descs[2], descs[3]);
>  	vtag0 = _mm_unpackhi_epi16(descs[0], descs[1]);
> @@ -178,8 +184,8 @@ desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
>  	ptype0 = _mm_shuffle_epi8(rss_flags, ptype0);
> 
>  	vtag1 = _mm_unpacklo_epi32(vtag0, vtag1);
> -	vtag1 = _mm_srli_epi16(vtag1, VTAG_SHIFT);
> -	vtag1 = _mm_and_si128(vtag1, pkttype_msk);
> +	vtag1 = _mm_and_si128(vtag1, vlan_msk);
> +	vtag1 = _mm_shuffle_epi8(vlan_map, vtag1);
> 
>  	vtag1 = _mm_or_si128(ptype0, vtag1);
>  	vol.dword = _mm_cvtsi128_si64(vtag1);
> @@ -221,6 +227,7 @@ _recv_raw_pkts_vec(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_pkts,
>  				0, 0            /* ignore pkt_type field */
>  			);
>  	__m128i dd_check, eop_check;
> +	uint8_t vlan_flags;
> 
>  	/* nb_pkts shall be less equal than RTE_IXGBE_MAX_RX_BURST */
>  	nb_pkts = RTE_MIN(nb_pkts, RTE_IXGBE_MAX_RX_BURST);
> @@ -270,6 +277,11 @@ _recv_raw_pkts_vec(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_pkts,
>  	 */
>  	sw_ring = &rxq->sw_ring[rxq->rx_tail];
> 
> +	/* ensure these 2 flags are in the lower 8 bits */
> +	RTE_BUILD_BUG_ON(((PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED) &
> +			0xffffffffffffff00ULL) != 0ULL);


I suppose your need here:
RTE_BUILD_BUG_ON(((PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED) & UINT8_MAX) == 
PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED));

To make sure both flags are inside first 8 bits?

			
> +	vlan_flags = rxq->vlan_flags & 0xff;
> +

Probably better to do that check/ AND at setup  phase, not run-time?
As a nit: s/0xff/UINT8_MAX/.
Konstantin