[dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in rte_mbuf

[O'Driscoll, Tim]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Olivier MATZ
> Sent: Monday, June 1, 2015 9:15 AM
> To: Zhang, Helin; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
> rte_mbuf
> 
> Hi Helin,
> 
> +CC Neil
> 
> On 06/01/2015 09:33 AM, Helin Zhang wrote:
> > 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.
> > To avoid breaking ABI compatibility, all the changes would be
> > enabled by RTE_UNIFIED_PKT_TYPE, which is disabled by default.
> 
> What are the plans for this compile-time option in the future?
> 
> I wonder what are the benefits of having this option in terms
> of ABI compatibility: when it is disabled, it is ABI-compatible but
> the packet-type feature is not present, and when it is enabled we
> have the feature but it breaks the compatibility.
> 
> In my opinion, the v5 is preferable: for this kind of features, I
> don't see how the ABI can be preserved, and I think packet-type
> won't be the only feature that will modify the mbuf structure. I think
> the process described here should be applied:
> http://dpdk.org/browse/dpdk/tree/doc/guides/rel_notes/abi.rst
> 
> (starting from "Some ABI changes may be too significant to reasonably
> maintain multiple versions of").
> 
> 
> Regards,
> Olivier
> 

This is just like the change that Steve (Cunming) Liang submitted for Interrupt Mode. We have the same problem in both cases: we want to find a way to get the features included, but need to comply with our ABI policy. So, in both cases, the proposal is to add a config option to enable the change by default, so we maintain backward compatibility. Users that want these changes, and are willing to accept the associated ABI change, have to specifically enable them.

We can note in the Deprecation Notices in the Release Notes for 2.1 that these config options will be removed in 2.2. The features will then be enabled by default.

This seems like a good compromise which allows us to get these changes into 2.1 but avoids breaking the ABI policy.


Tim

> 
> 
> >
> > Signed-off-by: Helin Zhang <helin.zhang at intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang at intel.com>
> > ---
> >  config/common_linuxapp                             |  2 +-
> >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  6 ++++++
> >  lib/librte_mbuf/rte_mbuf.h                         | 23
> ++++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> >
> > 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.
> >
> > v5 changes:
> > * Re-worded the commit logs.
> >
> > v6 changes:
> > * Disabled the code changes for unified packet type by default, to
> >   avoid breaking ABI compatibility.
> >
> > diff --git a/config/common_linuxapp b/config/common_linuxapp
> > index 0078dc9..6b067c7 100644
> > --- a/config/common_linuxapp
> > +++ b/config/common_linuxapp
> > @@ -167,7 +167,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..7a2abbb 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,15 @@ struct rte_kni_mbuf {
> >  	uint16_t data_off;      /**< Start address of data in segment
> buffer. */
> >  	char pad1[4];
> >  	uint64_t ol_flags;      /**< Offload features. */
> > +#ifdef RTE_UNIFIED_PKT_TYPE
> > +	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. */
> > +#else
> >  	char pad2[2];
> >  	uint16_t data_len;      /**< Amount of data in segment buffer. */
> >  	uint32_t pkt_len;       /**< Total pkt len: sum of all segment
> data_len. */
> > +#endif
> >
> >  	/* 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 ab6de67..a8662c2 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -269,6 +269,28 @@ struct rte_mbuf {
> >  	/* remaining bytes are set on RX when pulling packet from
> descriptor */
> >  	MARKER rx_descriptor_fields1;
> >
> > +#ifdef RTE_UNIFIED_PKT_TYPE
> > +	/*
> > +	 * The packet type, which is the combination of outer/inner L2, L3,
> L4
> > +	 * and tunnel types.
> > +	 */
> > +	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. */
> > +		};
> > +	};
> > +
> > +	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) */
> > +#else
> >  	/**
> >  	 * The packet type, which is used to indicate ordinary packet and
> also
> >  	 * tunneled packet format, i.e. each number is represented a type
> of
> > @@ -280,6 +302,7 @@ struct rte_mbuf {
> >  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments.
> */
> >  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU
> order) */
> >  	uint16_t reserved;
> > +#endif
> >  	union {
> >  		uint32_t rss;     /**< RSS hash result if RSS enabled */
> >  		struct {
> >