Re: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type in rte_mbuf

["Chilikin, Andrey" <andrey.chilikin@intel.com>]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Wednesday, March 4, 2015 8:34 AM
> To: Chilikin, Andrey; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type
> in rte_mbuf
> 
> 
> 
> > -----Original Message-----
> > From: Chilikin, Andrey
> > Sent: Monday, March 2, 2015 7:48 PM
> > To: Zhang, Helin; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of
> > packet_type in rte_mbuf
> >
> > Hi Helin,
> >
> > I see that you have removed "uint16_t reserved" member from rte_mbuf:
> >
> > > +	uint16_t data_len;        /**< Amount of data in segment buffer. */
> > >  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order)
> > */
> > > -	uint16_t reserved;
> > >  	union {
> > >  		uint32_t rss;     /**< RSS hash result if RSS enabled */
> >
> > This reserved field was kept next to  vlan_tci as a placeholder for
> > the second VLAN label for QinQ support so if need be vlan_tci +
> > reserved could be casted to
> > 32 bit QinQ value or one 32bit VNTAG label. Without keeping two label
> > adjusted to each other casting to 32 bit will not be possible and will
> > affect QinQ performance.
> Yes, but packet type is quite important which needs to be extended from 16
> bits to 32 bits.
> For FVL, the vlan tags are in different fields. 
I do not see how FVL internal descriptor can affect DPDK mbuf structure.

> We can think of putting them
> together in mbuf, Possibly move current vlan tag down, and add one more 16
> bits. Let's see what is the best then.
But if we know that we would need this change for QinQ anyway should we move
vlan_tci +reserved 16bits now in this patch instead of testing performance twice -
for this and for future patch when we move vlan_tci+reserved?

Regards,
Andrey

> Thanks for the notes!
> 
> Regards,
> Helin
> 
> >
> > Regards,
> > Andrey
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> > > Sent: Friday, February 27, 2015 1:11 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of
> > > packet_type in rte_mbuf
> > >
> > > In order to unify the packet type, the field of 'packet_type' in
> > > 'struct rte_mbuf' needs to be extended from 16 to 32 bits.
> > > Accordingly, some fields in 'struct rte_mbuf' are re-organized to
> > > support this change for Vector PMD. As 'struct rte_kni_mbuf' for KNI
> > > should be right mapped to 'struct rte_mbuf', it should be modified
> > > accordingly. In addition, Vector PMD of ixgbe is disabled by
> > > default, as 'struct
> > rte_mbuf' changed.
> > >
> > > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > ---
> > >  config/common_linuxapp                             |  2 +-
> > >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  4 ++--
> > >  lib/librte_mbuf/rte_mbuf.h                         | 23
> > +++++++++++++++-------
> > >  3 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > v2 changes:
> > > * Enlarged the packet_type field from 16 bits to 32 bits.
> > > * Redefined the packet type sub-fields.
> > > * Updated the 'struct rte_kni_mbuf' for KNI according to the mbuf
> changes.
> > >
> > > v3 changes:
> > > * Put the mbuf layout changes into a single patch.
> > > * Disabled vector ixgbe PMD by default, as mbuf layout changed.
> > >
> > > diff --git a/config/common_linuxapp b/config/common_linuxapp index
> > > 97f1c9e..97d7bae 100644
> > > --- a/config/common_linuxapp
> > > +++ b/config/common_linuxapp
> > > @@ -166,7 +166,7 @@ CONFIG_RTE_LIBRTE_IXGBE_DEBUG_TX_FREE=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC=y
> > > -CONFIG_RTE_IXGBE_INC_VECTOR=y
> > > +CONFIG_RTE_IXGBE_INC_VECTOR=n
> > >  CONFIG_RTE_IXGBE_RX_OLFLAGS_ENABLE=y
> > >
> > >  #
> > > diff --git
> > > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > index 1e55c2d..bd1cc09 100644
> > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > @@ -117,9 +117,9 @@ struct rte_kni_mbuf {
> > >  	uint16_t data_off;      /**< Start address of data in segment buffer. */
> > >  	char pad1[4];
> > >  	uint64_t ol_flags;      /**< Offload features. */
> > > -	char pad2[2];
> > > -	uint16_t data_len;      /**< Amount of data in segment buffer. */
> > > +	char pad2[4];
> > >  	uint32_t pkt_len;       /**< Total pkt len: sum of all segment data_len.
> > > */
> > > +	uint16_t data_len;      /**< Amount of data in segment buffer. */
> > >
> > >  	/* fields on second cache line */
> > >  	char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 17ba791..f5b7a8b 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -258,17 +258,26 @@ struct rte_mbuf {
> > >  	/* remaining bytes are set on RX when pulling packet from
> > > descriptor */
> > >  	MARKER rx_descriptor_fields1;
> > >
> > > -	/**
> > > -	 * The packet type, which is used to indicate ordinary packet and also
> > > -	 * tunneled packet format, i.e. each number is represented a type of
> > > -	 * packet.
> > > +	/*
> > > +	 * The packet type, which is the combination of outer/inner L2, L3, L4
> > > +	 * and tunnel types.
> > >  	 */
> > > -	uint16_t packet_type;
> > > +	union {
> > > +		uint32_t packet_type; /**< L2/L3/L4 and tunnel information.
> > > */
> > > +		struct {
> > > +			uint32_t l2_type:4; /**< (Outer) L2 type. */
> > > +			uint32_t l3_type:4; /**< (Outer) L3 type. */
> > > +			uint32_t l4_type:4; /**< (Outer) L4 type. */
> > > +			uint32_t tun_type:4; /**< Tunnel type. */
> > > +			uint32_t inner_l2_type:4; /**< Inner L2 type. */
> > > +			uint32_t inner_l3_type:4; /**< Inner L3 type. */
> > > +			uint32_t inner_l4_type:4; /**< Inner L4 type. */
> > > +		};
> > > +	};
> > >
> > > -	uint16_t data_len;        /**< Amount of data in segment buffer. */
> > >  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
> > > +	uint16_t data_len;        /**< Amount of data in segment buffer. */
> > >  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order)
> > */
> > > -	uint16_t reserved;
> > >  	union {
> > >  		uint32_t rss;     /**< RSS hash result if RSS enabled */
> > >  		struct {
> > > --
> > > 1.9.3