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

Message ID 1433144045-30847-2-git-send-email-helin.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Zhang, Helin June 1, 2015, 7:33 a.m. UTC
  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.

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 |  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.
  

Comments

Olivier Matz June 1, 2015, 8:14 a.m. UTC | #1
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



> 
> 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 |  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 {
>
  
O'driscoll, Tim June 2, 2015, 1:27 p.m. UTC | #2
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Monday, June 1, 2015 9:15 AM
> To: Zhang, Helin; dev@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@intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang@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 {
> >
  
Olivier Matz June 10, 2015, 2:32 p.m. UTC | #3
Hi Tim, Helin,

On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
>> Sent: Monday, June 1, 2015 9:15 AM
>> To: Zhang, Helin; dev@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.

Sorry for the late answer.

After some thoughts on this topic, I understand that having a
compile-time option is perhaps a good compromise between
keeping compatibility and having new features earlier.

I'm just afraid about having one #ifdef in the code for
each new feature that cannot keep the ABI compatibility.
What do you think about having one option -- let's call
it "CONFIG_RTE_NEXT_ABI" --, that is disabled by default,
and that would surround any new feature that breaks the
ABI?

This would have several advantages:
- only 2 cases (on or off), the combinatorial is smaller than
   having one option per feature
- all next features breaking the abi can be identified by a grep
- the code inside the #ifdef can be enabled in a simple operation
   by Thomas after each release.

Thomas, any comment?

Regards,
Olivier
  
Zhang, Helin June 10, 2015, 2:51 p.m. UTC | #4
Hi Oliver

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, June 10, 2015 10:33 PM
> To: O'Driscoll, Tim; Zhang, Helin; dev@dpdk.org
> Cc: Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
> rte_mbuf
> 
> Hi Tim, Helin,
> 
> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> >> Sent: Monday, June 1, 2015 9:15 AM
> >> To: Zhang, Helin; dev@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.
> 
> Sorry for the late answer.
> 
> After some thoughts on this topic, I understand that having a compile-time
> option is perhaps a good compromise between keeping compatibility and having
> new features earlier.
> 
> I'm just afraid about having one #ifdef in the code for each new feature that
> cannot keep the ABI compatibility.
> What do you think about having one option -- let's call it
> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that would surround
> any new feature that breaks the ABI?
Will we allow this type of workaround for a long time? If yes, agree with your good idea.

Regards,
Helin

> 
> This would have several advantages:
> - only 2 cases (on or off), the combinatorial is smaller than
>    having one option per feature
> - all next features breaking the abi can be identified by a grep
> - the code inside the #ifdef can be enabled in a simple operation
>    by Thomas after each release.
> 
> Thomas, any comment?
> 
> Regards,
> Olivier
>
  
Ananyev, Konstantin June 10, 2015, 3:39 p.m. UTC | #5
Hi Olivier,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Wednesday, June 10, 2015 3:33 PM
> To: O'Driscoll, Tim; Zhang, Helin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in rte_mbuf
> 
> Hi Tim, Helin,
> 
> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> >> Sent: Monday, June 1, 2015 9:15 AM
> >> To: Zhang, Helin; dev@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.
> 
> Sorry for the late answer.
> 
> After some thoughts on this topic, I understand that having a
> compile-time option is perhaps a good compromise between
> keeping compatibility and having new features earlier.
> 
> I'm just afraid about having one #ifdef in the code for
> each new feature that cannot keep the ABI compatibility.
> What do you think about having one option -- let's call
> it "CONFIG_RTE_NEXT_ABI" --, that is disabled by default,
> and that would surround any new feature that breaks the
> ABI?

I am not Tim/Helin, but really like that idea :)
Konstantin


> 
> This would have several advantages:
> - only 2 cases (on or off), the combinatorial is smaller than
>    having one option per feature
> - all next features breaking the abi can be identified by a grep
> - the code inside the #ifdef can be enabled in a simple operation
>    by Thomas after each release.
> 
> Thomas, any comment?
> 
> Regards,
> Olivier
>
  
Thomas Monjalon June 10, 2015, 4:14 p.m. UTC | #6
2015-06-10 16:32, Olivier MATZ:
> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> >> 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").
> >
> > 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.
> 
> Sorry for the late answer.
> 
> After some thoughts on this topic, I understand that having a
> compile-time option is perhaps a good compromise between
> keeping compatibility and having new features earlier.
> 
> I'm just afraid about having one #ifdef in the code for
> each new feature that cannot keep the ABI compatibility.
> What do you think about having one option -- let's call
> it "CONFIG_RTE_NEXT_ABI" --, that is disabled by default,
> and that would surround any new feature that breaks the
> ABI?
> 
> This would have several advantages:
> - only 2 cases (on or off), the combinatorial is smaller than
>    having one option per feature
> - all next features breaking the abi can be identified by a grep
> - the code inside the #ifdef can be enabled in a simple operation
>    by Thomas after each release.
> 
> Thomas, any comment?

As previously discussed (1to1) with Olivier, I think that's a good proposal
to introduce changes breaking deeply the ABI.

Let's sum up the current policy:
1/ For changes which have a limited impact on the ABI, the backward compatibility
must be kept during 1 release including the notice in doc/guides/rel_notes/abi.rst.
2/ For important changes like mbuf rework, there was an agreement on skipping the
backward compatibility after having 3 acknowledgements and an 1-release long notice.
Then the ABI numbering must be incremented.

This CONFIG_RTE_NEXT_ABI proposal would change the rules for the second case.
In order to be adopted, a patch for the file doc/guides/rel_notes/abi.rst must
be submitted and strongly acknowledged.

The ABI numbering must be also clearly explained:
1/ Should we have different libraries version number depending of CONFIG_RTE_NEXT_ABI?
It seems straightforward to use "ifeq" when LIBABIVER in the Makefiles
2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in the .map files?
Maybe we should remove these files and generate them with some preprocessing.

Neil, as the ABI policy author, what is your opinion?
  
Zhang, Helin June 12, 2015, 3:22 a.m. UTC | #7
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, June 10, 2015 11:40 PM
> To: Olivier MATZ; O'Driscoll, Tim; Zhang, Helin; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
> rte_mbuf
> 
> Hi Olivier,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > Sent: Wednesday, June 10, 2015 3:33 PM
> > To: O'Driscoll, Tim; Zhang, Helin; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
> > rte_mbuf
> >
> > Hi Tim, Helin,
> >
> > On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
> > >
> > >> -----Original Message-----
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > >> Sent: Monday, June 1, 2015 9:15 AM
> > >> To: Zhang, Helin; dev@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.
> >
> > Sorry for the late answer.
> >
> > After some thoughts on this topic, I understand that having a
> > compile-time option is perhaps a good compromise between keeping
> > compatibility and having new features earlier.
> >
> > I'm just afraid about having one #ifdef in the code for each new
> > feature that cannot keep the ABI compatibility.
> > What do you think about having one option -- let's call it
> > "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that would
> > surround any new feature that breaks the ABI?
> 
> I am not Tim/Helin, but really like that idea :) Konstantin

It seems more guys like Oliver's idea of introducing CONFIG_RTE_NEXT_ABI. Any objections?
If none, I will rework my patches with that.

- Helin

> 
> 
> >
> > This would have several advantages:
> > - only 2 cases (on or off), the combinatorial is smaller than
> >    having one option per feature
> > - all next features breaking the abi can be identified by a grep
> > - the code inside the #ifdef can be enabled in a simple operation
> >    by Thomas after each release.
> >
> > Thomas, any comment?
> >
> > Regards,
> > Olivier
> >
  
Panu Matilainen June 12, 2015, 7:24 a.m. UTC | #8
On 06/10/2015 07:14 PM, Thomas Monjalon wrote:
> 2015-06-10 16:32, Olivier MATZ:
>> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
>>>> 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").
>>>
>>> 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.
>>
>> Sorry for the late answer.
>>
>> After some thoughts on this topic, I understand that having a
>> compile-time option is perhaps a good compromise between
>> keeping compatibility and having new features earlier.
>>
>> I'm just afraid about having one #ifdef in the code for
>> each new feature that cannot keep the ABI compatibility.
>> What do you think about having one option -- let's call
>> it "CONFIG_RTE_NEXT_ABI" --, that is disabled by default,
>> and that would surround any new feature that breaks the
>> ABI?
>>
>> This would have several advantages:
>> - only 2 cases (on or off), the combinatorial is smaller than
>>     having one option per feature
>> - all next features breaking the abi can be identified by a grep
>> - the code inside the #ifdef can be enabled in a simple operation
>>     by Thomas after each release.
>>
>> Thomas, any comment?
>
> As previously discussed (1to1) with Olivier, I think that's a good proposal
> to introduce changes breaking deeply the ABI.
>
> Let's sum up the current policy:
> 1/ For changes which have a limited impact on the ABI, the backward compatibility
> must be kept during 1 release including the notice in doc/guides/rel_notes/abi.rst.
> 2/ For important changes like mbuf rework, there was an agreement on skipping the
> backward compatibility after having 3 acknowledgements and an 1-release long notice.
> Then the ABI numbering must be incremented.
>
> This CONFIG_RTE_NEXT_ABI proposal would change the rules for the second case.
> In order to be adopted, a patch for the file doc/guides/rel_notes/abi.rst must
> be submitted and strongly acknowledged.
>
> The ABI numbering must be also clearly explained:
> 1/ Should we have different libraries version number depending of CONFIG_RTE_NEXT_ABI?
> It seems straightforward to use "ifeq" when LIBABIVER in the Makefiles

An incompatible ABI must be reflected by a soname change, otherwise the 
whole library versioning is irrelevant.

> 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in the .map files?
> Maybe we should remove these files and generate them with some preprocessing.
>
> Neil, as the ABI policy author, what is your opinion?

I'm not Neil but my 5c...

Working around ABI compatibility policy via config options seems like a 
slippery slope. Going forward this will likely mean there are always two 
different ABIs for any given version, and the thought of keeping track 
of it all in a truly compatible manner makes my head hurt.

That said its easy to understand the desire to move faster than the ABI 
policy allows. In a project where so many structs are in the open it 
gets hard to do much anything at all without breaking the ABI.

The issue could be mitigated somewhat by reserving some space at the end 
of the structs eg when the ABI needs to be changed anyway, but it has 
obvious downsides as well. The other options I see tend to revolve 
around changing release policies one way or the other: releasing ABI 
compatible micro versions between minor versions and relaxing the ABI 
policy a bit, or just releasing new minor versions more often than the 
current cycle.

	- Panu -
  
Zhang, Helin June 12, 2015, 7:43 a.m. UTC | #9
> -----Original Message-----

> From: Panu Matilainen [mailto:pmatilai@redhat.com]

> Sent: Friday, June 12, 2015 3:24 PM

> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;

> nhorman@tuxdriver.com

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in

> rte_mbuf

> 

> On 06/10/2015 07:14 PM, Thomas Monjalon wrote:

> > 2015-06-10 16:32, Olivier MATZ:

> >> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:

> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ

> >>>> 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").

> >>>

> >>> 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.

> >>

> >> Sorry for the late answer.

> >>

> >> After some thoughts on this topic, I understand that having a

> >> compile-time option is perhaps a good compromise between keeping

> >> compatibility and having new features earlier.

> >>

> >> I'm just afraid about having one #ifdef in the code for each new

> >> feature that cannot keep the ABI compatibility.

> >> What do you think about having one option -- let's call it

> >> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that would

> >> surround any new feature that breaks the ABI?

> >>

> >> This would have several advantages:

> >> - only 2 cases (on or off), the combinatorial is smaller than

> >>     having one option per feature

> >> - all next features breaking the abi can be identified by a grep

> >> - the code inside the #ifdef can be enabled in a simple operation

> >>     by Thomas after each release.

> >>

> >> Thomas, any comment?

> >

> > As previously discussed (1to1) with Olivier, I think that's a good

> > proposal to introduce changes breaking deeply the ABI.

> >

> > Let's sum up the current policy:

> > 1/ For changes which have a limited impact on the ABI, the backward

> > compatibility must be kept during 1 release including the notice in

> doc/guides/rel_notes/abi.rst.

> > 2/ For important changes like mbuf rework, there was an agreement on

> > skipping the backward compatibility after having 3 acknowledgements and an

> 1-release long notice.

> > Then the ABI numbering must be incremented.

> >

> > This CONFIG_RTE_NEXT_ABI proposal would change the rules for the second

> case.

> > In order to be adopted, a patch for the file

> > doc/guides/rel_notes/abi.rst must be submitted and strongly acknowledged.

> >

> > The ABI numbering must be also clearly explained:

> > 1/ Should we have different libraries version number depending of

> CONFIG_RTE_NEXT_ABI?

> > It seems straightforward to use "ifeq" when LIBABIVER in the Makefiles

> 

> An incompatible ABI must be reflected by a soname change, otherwise the

> whole library versioning is irrelevant.

> 

> > 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in

> the .map files?

> > Maybe we should remove these files and generate them with some

> preprocessing.

> >

> > Neil, as the ABI policy author, what is your opinion?

> 

> I'm not Neil but my 5c...

> 

> Working around ABI compatibility policy via config options seems like a slippery

> slope. Going forward this will likely mean there are always two different ABIs for

> any given version, and the thought of keeping track of it all in a truly compatible

> manner makes my head hurt.

> 

> That said its easy to understand the desire to move faster than the ABI policy

> allows. In a project where so many structs are in the open it gets hard to do much

> anything at all without breaking the ABI.

> 

> The issue could be mitigated somewhat by reserving some space at the end of

> the structs eg when the ABI needs to be changed anyway, but it has obvious

> downsides as well. The other options I see tend to revolve around changing

> release policies one way or the other: releasing ABI compatible micro versions

> between minor versions and relaxing the ABI policy a bit, or just releasing new

> minor versions more often than the current cycle.

> 

> 	- Panu -


Does it mean releasing R2.01 right now with announcement of all ABI changes, which
based on R2.0 first, and then releasing R2.1 several weeks later with all the code changes?

- Helin
  
Panu Matilainen June 12, 2015, 8:15 a.m. UTC | #10
On 06/12/2015 10:43 AM, Zhang, Helin wrote:
>
>
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>> Sent: Friday, June 12, 2015 3:24 PM
>> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;
>> nhorman@tuxdriver.com
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
>> rte_mbuf
>>
>> On 06/10/2015 07:14 PM, Thomas Monjalon wrote:
>>> 2015-06-10 16:32, Olivier MATZ:
>>>> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
>>>>>> 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").
>>>>>
>>>>> 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.
>>>>
>>>> Sorry for the late answer.
>>>>
>>>> After some thoughts on this topic, I understand that having a
>>>> compile-time option is perhaps a good compromise between keeping
>>>> compatibility and having new features earlier.
>>>>
>>>> I'm just afraid about having one #ifdef in the code for each new
>>>> feature that cannot keep the ABI compatibility.
>>>> What do you think about having one option -- let's call it
>>>> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that would
>>>> surround any new feature that breaks the ABI?
>>>>
>>>> This would have several advantages:
>>>> - only 2 cases (on or off), the combinatorial is smaller than
>>>>      having one option per feature
>>>> - all next features breaking the abi can be identified by a grep
>>>> - the code inside the #ifdef can be enabled in a simple operation
>>>>      by Thomas after each release.
>>>>
>>>> Thomas, any comment?
>>>
>>> As previously discussed (1to1) with Olivier, I think that's a good
>>> proposal to introduce changes breaking deeply the ABI.
>>>
>>> Let's sum up the current policy:
>>> 1/ For changes which have a limited impact on the ABI, the backward
>>> compatibility must be kept during 1 release including the notice in
>> doc/guides/rel_notes/abi.rst.
>>> 2/ For important changes like mbuf rework, there was an agreement on
>>> skipping the backward compatibility after having 3 acknowledgements and an
>> 1-release long notice.
>>> Then the ABI numbering must be incremented.
>>>
>>> This CONFIG_RTE_NEXT_ABI proposal would change the rules for the second
>> case.
>>> In order to be adopted, a patch for the file
>>> doc/guides/rel_notes/abi.rst must be submitted and strongly acknowledged.
>>>
>>> The ABI numbering must be also clearly explained:
>>> 1/ Should we have different libraries version number depending of
>> CONFIG_RTE_NEXT_ABI?
>>> It seems straightforward to use "ifeq" when LIBABIVER in the Makefiles
>>
>> An incompatible ABI must be reflected by a soname change, otherwise the
>> whole library versioning is irrelevant.
>>
>>> 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in
>> the .map files?
>>> Maybe we should remove these files and generate them with some
>> preprocessing.
>>>
>>> Neil, as the ABI policy author, what is your opinion?
>>
>> I'm not Neil but my 5c...
>>
>> Working around ABI compatibility policy via config options seems like a slippery
>> slope. Going forward this will likely mean there are always two different ABIs for
>> any given version, and the thought of keeping track of it all in a truly compatible
>> manner makes my head hurt.
>>
>> That said its easy to understand the desire to move faster than the ABI policy
>> allows. In a project where so many structs are in the open it gets hard to do much
>> anything at all without breaking the ABI.
>>
>> The issue could be mitigated somewhat by reserving some space at the end of
>> the structs eg when the ABI needs to be changed anyway, but it has obvious
>> downsides as well. The other options I see tend to revolve around changing
>> release policies one way or the other: releasing ABI compatible micro versions
>> between minor versions and relaxing the ABI policy a bit, or just releasing new
>> minor versions more often than the current cycle.
>>
>> 	- Panu -
>
> Does it mean releasing R2.01 right now with announcement of all ABI changes, which
> based on R2.0 first, and then releasing R2.1 several weeks later with all the code changes?

Something like that, but I'd think its too late for any big release 
model / policy changes for this particular cycle.

I also do not want to undermine the ABI policy we just got in place, but 
since people are actively looking for ways to work around it anyway its 
better to map out all the possibilities. One of them is committing to 
longer term maintenance of releases (via ABI compatible micro version 
updates), another one is shortening the cycles. Both achieve roughly the 
same goals with differences in emphasis perhaps, but more releases 
requires more resources on maintaining, testing etc so...

	- Panu -
  
Zhang, Helin June 12, 2015, 8:28 a.m. UTC | #11
> -----Original Message-----

> From: Panu Matilainen [mailto:pmatilai@redhat.com]

> Sent: Friday, June 12, 2015 4:15 PM

> To: Zhang, Helin; Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim;

> nhorman@tuxdriver.com

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in

> rte_mbuf

> 

> On 06/12/2015 10:43 AM, Zhang, Helin wrote:

> >

> >

> >> -----Original Message-----

> >> From: Panu Matilainen [mailto:pmatilai@redhat.com]

> >> Sent: Friday, June 12, 2015 3:24 PM

> >> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;

> >> nhorman@tuxdriver.com

> >> Cc: dev@dpdk.org

> >> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type

> >> in rte_mbuf

> >>

> >> On 06/10/2015 07:14 PM, Thomas Monjalon wrote:

> >>> 2015-06-10 16:32, Olivier MATZ:

> >>>> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:

> >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ

> >>>>>> 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").

> >>>>>

> >>>>> 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.

> >>>>

> >>>> Sorry for the late answer.

> >>>>

> >>>> After some thoughts on this topic, I understand that having a

> >>>> compile-time option is perhaps a good compromise between keeping

> >>>> compatibility and having new features earlier.

> >>>>

> >>>> I'm just afraid about having one #ifdef in the code for each new

> >>>> feature that cannot keep the ABI compatibility.

> >>>> What do you think about having one option -- let's call it

> >>>> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that

> >>>> would surround any new feature that breaks the ABI?

> >>>>

> >>>> This would have several advantages:

> >>>> - only 2 cases (on or off), the combinatorial is smaller than

> >>>>      having one option per feature

> >>>> - all next features breaking the abi can be identified by a grep

> >>>> - the code inside the #ifdef can be enabled in a simple operation

> >>>>      by Thomas after each release.

> >>>>

> >>>> Thomas, any comment?

> >>>

> >>> As previously discussed (1to1) with Olivier, I think that's a good

> >>> proposal to introduce changes breaking deeply the ABI.

> >>>

> >>> Let's sum up the current policy:

> >>> 1/ For changes which have a limited impact on the ABI, the backward

> >>> compatibility must be kept during 1 release including the notice in

> >> doc/guides/rel_notes/abi.rst.

> >>> 2/ For important changes like mbuf rework, there was an agreement on

> >>> skipping the backward compatibility after having 3 acknowledgements

> >>> and an

> >> 1-release long notice.

> >>> Then the ABI numbering must be incremented.

> >>>

> >>> This CONFIG_RTE_NEXT_ABI proposal would change the rules for the

> >>> second

> >> case.

> >>> In order to be adopted, a patch for the file

> >>> doc/guides/rel_notes/abi.rst must be submitted and strongly

> acknowledged.

> >>>

> >>> The ABI numbering must be also clearly explained:

> >>> 1/ Should we have different libraries version number depending of

> >> CONFIG_RTE_NEXT_ABI?

> >>> It seems straightforward to use "ifeq" when LIBABIVER in the

> >>> Makefiles

> >>

> >> An incompatible ABI must be reflected by a soname change, otherwise

> >> the whole library versioning is irrelevant.

> >>

> >>> 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in

> >> the .map files?

> >>> Maybe we should remove these files and generate them with some

> >> preprocessing.

> >>>

> >>> Neil, as the ABI policy author, what is your opinion?

> >>

> >> I'm not Neil but my 5c...

> >>

> >> Working around ABI compatibility policy via config options seems like

> >> a slippery slope. Going forward this will likely mean there are

> >> always two different ABIs for any given version, and the thought of

> >> keeping track of it all in a truly compatible manner makes my head hurt.

> >>

> >> That said its easy to understand the desire to move faster than the

> >> ABI policy allows. In a project where so many structs are in the open

> >> it gets hard to do much anything at all without breaking the ABI.

> >>

> >> The issue could be mitigated somewhat by reserving some space at the

> >> end of the structs eg when the ABI needs to be changed anyway, but it

> >> has obvious downsides as well. The other options I see tend to

> >> revolve around changing release policies one way or the other:

> >> releasing ABI compatible micro versions between minor versions and

> >> relaxing the ABI policy a bit, or just releasing new minor versions more often

> than the current cycle.

> >>

> >> 	- Panu -

> >

> > Does it mean releasing R2.01 right now with announcement of all ABI

> > changes, which based on R2.0 first, and then releasing R2.1 several weeks later

> with all the code changes?

> 

> Something like that, but I'd think its too late for any big release model / policy

> changes for this particular cycle.

> 

> I also do not want to undermine the ABI policy we just got in place, but since

> people are actively looking for ways to work around it anyway its better to map

> out all the possibilities. One of them is committing to longer term maintenance of

> releases (via ABI compatible micro version updates), another one is shortening

> the cycles. Both achieve roughly the same goals with differences in emphasis

> perhaps, but more releases requires more resources on maintaining, testing etc

> so...

R2.01 could just have all the same of R2.0, with an additional ABI announcement.
Then nothing needs to be tested.

- Helin

> 

> 	- Panu -
  
Panu Matilainen June 12, 2015, 9 a.m. UTC | #12
On 06/12/2015 11:28 AM, Zhang, Helin wrote:
>
>
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>> Sent: Friday, June 12, 2015 4:15 PM
>> To: Zhang, Helin; Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim;
>> nhorman@tuxdriver.com
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
>> rte_mbuf
>>
>> On 06/12/2015 10:43 AM, Zhang, Helin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>>>> Sent: Friday, June 12, 2015 3:24 PM
>>>> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;
>>>> nhorman@tuxdriver.com
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type
>>>> in rte_mbuf
>>>>
>>>> On 06/10/2015 07:14 PM, Thomas Monjalon wrote:
>>>>> 2015-06-10 16:32, Olivier MATZ:
>>>>>> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
>>>>>>>> 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").
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Sorry for the late answer.
>>>>>>
>>>>>> After some thoughts on this topic, I understand that having a
>>>>>> compile-time option is perhaps a good compromise between keeping
>>>>>> compatibility and having new features earlier.
>>>>>>
>>>>>> I'm just afraid about having one #ifdef in the code for each new
>>>>>> feature that cannot keep the ABI compatibility.
>>>>>> What do you think about having one option -- let's call it
>>>>>> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that
>>>>>> would surround any new feature that breaks the ABI?
>>>>>>
>>>>>> This would have several advantages:
>>>>>> - only 2 cases (on or off), the combinatorial is smaller than
>>>>>>       having one option per feature
>>>>>> - all next features breaking the abi can be identified by a grep
>>>>>> - the code inside the #ifdef can be enabled in a simple operation
>>>>>>       by Thomas after each release.
>>>>>>
>>>>>> Thomas, any comment?
>>>>>
>>>>> As previously discussed (1to1) with Olivier, I think that's a good
>>>>> proposal to introduce changes breaking deeply the ABI.
>>>>>
>>>>> Let's sum up the current policy:
>>>>> 1/ For changes which have a limited impact on the ABI, the backward
>>>>> compatibility must be kept during 1 release including the notice in
>>>> doc/guides/rel_notes/abi.rst.
>>>>> 2/ For important changes like mbuf rework, there was an agreement on
>>>>> skipping the backward compatibility after having 3 acknowledgements
>>>>> and an
>>>> 1-release long notice.
>>>>> Then the ABI numbering must be incremented.
>>>>>
>>>>> This CONFIG_RTE_NEXT_ABI proposal would change the rules for the
>>>>> second
>>>> case.
>>>>> In order to be adopted, a patch for the file
>>>>> doc/guides/rel_notes/abi.rst must be submitted and strongly
>> acknowledged.
>>>>>
>>>>> The ABI numbering must be also clearly explained:
>>>>> 1/ Should we have different libraries version number depending of
>>>> CONFIG_RTE_NEXT_ABI?
>>>>> It seems straightforward to use "ifeq" when LIBABIVER in the
>>>>> Makefiles
>>>>
>>>> An incompatible ABI must be reflected by a soname change, otherwise
>>>> the whole library versioning is irrelevant.
>>>>
>>>>> 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in
>>>> the .map files?
>>>>> Maybe we should remove these files and generate them with some
>>>> preprocessing.
>>>>>
>>>>> Neil, as the ABI policy author, what is your opinion?
>>>>
>>>> I'm not Neil but my 5c...
>>>>
>>>> Working around ABI compatibility policy via config options seems like
>>>> a slippery slope. Going forward this will likely mean there are
>>>> always two different ABIs for any given version, and the thought of
>>>> keeping track of it all in a truly compatible manner makes my head hurt.
>>>>
>>>> That said its easy to understand the desire to move faster than the
>>>> ABI policy allows. In a project where so many structs are in the open
>>>> it gets hard to do much anything at all without breaking the ABI.
>>>>
>>>> The issue could be mitigated somewhat by reserving some space at the
>>>> end of the structs eg when the ABI needs to be changed anyway, but it
>>>> has obvious downsides as well. The other options I see tend to
>>>> revolve around changing release policies one way or the other:
>>>> releasing ABI compatible micro versions between minor versions and
>>>> relaxing the ABI policy a bit, or just releasing new minor versions more often
>> than the current cycle.
>>>>
>>>> 	- Panu -
>>>
>>> Does it mean releasing R2.01 right now with announcement of all ABI
>>> changes, which based on R2.0 first, and then releasing R2.1 several weeks later
>> with all the code changes?
>>
>> Something like that, but I'd think its too late for any big release model / policy
>> changes for this particular cycle.
>>
>> I also do not want to undermine the ABI policy we just got in place, but since
>> people are actively looking for ways to work around it anyway its better to map
>> out all the possibilities. One of them is committing to longer term maintenance of
>> releases (via ABI compatible micro version updates), another one is shortening
>> the cycles. Both achieve roughly the same goals with differences in emphasis
>> perhaps, but more releases requires more resources on maintaining, testing etc
>> so...
> R2.01 could just have all the same of R2.0, with an additional ABI announcement.
> Then nothing needs to be tested.

That's also entirely missing the point of having an ABI policy in the 
first place. Its purpose is not to force people to find loopholes in the 
policy but for the benefit of other developers building apps on top of DPDK.

	- Panu -
  
Bruce Richardson June 12, 2015, 9:07 a.m. UTC | #13
On Fri, Jun 12, 2015 at 08:28:55AM +0000, Zhang, Helin wrote:
> 
> 
> > -----Original Message-----
> > From: Panu Matilainen [mailto:pmatilai@redhat.com]
> > Sent: Friday, June 12, 2015 4:15 PM
> > To: Zhang, Helin; Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim;
> > nhorman@tuxdriver.com
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type in
> > rte_mbuf
> > 
> > On 06/12/2015 10:43 AM, Zhang, Helin wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> > >> Sent: Friday, June 12, 2015 3:24 PM
> > >> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;
> > >> nhorman@tuxdriver.com
> > >> Cc: dev@dpdk.org
> > >> Subject: Re: [dpdk-dev] [PATCH v6 01/18] mbuf: redefine packet_type
> > >> in rte_mbuf
> > >>
> > >> On 06/10/2015 07:14 PM, Thomas Monjalon wrote:
> > >>> 2015-06-10 16:32, Olivier MATZ:
> > >>>> On 06/02/2015 03:27 PM, O'Driscoll, Tim wrote:
> > >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > >>>>>> 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").
> > >>>>>
> > >>>>> 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.
> > >>>>
> > >>>> Sorry for the late answer.
> > >>>>
> > >>>> After some thoughts on this topic, I understand that having a
> > >>>> compile-time option is perhaps a good compromise between keeping
> > >>>> compatibility and having new features earlier.
> > >>>>
> > >>>> I'm just afraid about having one #ifdef in the code for each new
> > >>>> feature that cannot keep the ABI compatibility.
> > >>>> What do you think about having one option -- let's call it
> > >>>> "CONFIG_RTE_NEXT_ABI" --, that is disabled by default, and that
> > >>>> would surround any new feature that breaks the ABI?
> > >>>>
> > >>>> This would have several advantages:
> > >>>> - only 2 cases (on or off), the combinatorial is smaller than
> > >>>>      having one option per feature
> > >>>> - all next features breaking the abi can be identified by a grep
> > >>>> - the code inside the #ifdef can be enabled in a simple operation
> > >>>>      by Thomas after each release.
> > >>>>
> > >>>> Thomas, any comment?
> > >>>
> > >>> As previously discussed (1to1) with Olivier, I think that's a good
> > >>> proposal to introduce changes breaking deeply the ABI.
> > >>>
> > >>> Let's sum up the current policy:
> > >>> 1/ For changes which have a limited impact on the ABI, the backward
> > >>> compatibility must be kept during 1 release including the notice in
> > >> doc/guides/rel_notes/abi.rst.
> > >>> 2/ For important changes like mbuf rework, there was an agreement on
> > >>> skipping the backward compatibility after having 3 acknowledgements
> > >>> and an
> > >> 1-release long notice.
> > >>> Then the ABI numbering must be incremented.
> > >>>
> > >>> This CONFIG_RTE_NEXT_ABI proposal would change the rules for the
> > >>> second
> > >> case.
> > >>> In order to be adopted, a patch for the file
> > >>> doc/guides/rel_notes/abi.rst must be submitted and strongly
> > acknowledged.
> > >>>
> > >>> The ABI numbering must be also clearly explained:
> > >>> 1/ Should we have different libraries version number depending of
> > >> CONFIG_RTE_NEXT_ABI?
> > >>> It seems straightforward to use "ifeq" when LIBABIVER in the
> > >>> Makefiles
> > >>
> > >> An incompatible ABI must be reflected by a soname change, otherwise
> > >> the whole library versioning is irrelevant.
> > >>
> > >>> 2/ Are we able to have some "if CONFIG_RTE_NEXT_ABI" statement in
> > >> the .map files?
> > >>> Maybe we should remove these files and generate them with some
> > >> preprocessing.
> > >>>
> > >>> Neil, as the ABI policy author, what is your opinion?
> > >>
> > >> I'm not Neil but my 5c...
> > >>
> > >> Working around ABI compatibility policy via config options seems like
> > >> a slippery slope. Going forward this will likely mean there are
> > >> always two different ABIs for any given version, and the thought of
> > >> keeping track of it all in a truly compatible manner makes my head hurt.
> > >>
> > >> That said its easy to understand the desire to move faster than the
> > >> ABI policy allows. In a project where so many structs are in the open
> > >> it gets hard to do much anything at all without breaking the ABI.
> > >>
> > >> The issue could be mitigated somewhat by reserving some space at the
> > >> end of the structs eg when the ABI needs to be changed anyway, but it
> > >> has obvious downsides as well. The other options I see tend to
> > >> revolve around changing release policies one way or the other:
> > >> releasing ABI compatible micro versions between minor versions and
> > >> relaxing the ABI policy a bit, or just releasing new minor versions more often
> > than the current cycle.
> > >>
> > >> 	- Panu -
> > >
> > > Does it mean releasing R2.01 right now with announcement of all ABI
> > > changes, which based on R2.0 first, and then releasing R2.1 several weeks later
> > with all the code changes?
> > 
> > Something like that, but I'd think its too late for any big release model / policy
> > changes for this particular cycle.
> > 
> > I also do not want to undermine the ABI policy we just got in place, but since
> > people are actively looking for ways to work around it anyway its better to map
> > out all the possibilities. One of them is committing to longer term maintenance of
> > releases (via ABI compatible micro version updates), another one is shortening
> > the cycles. Both achieve roughly the same goals with differences in emphasis
> > perhaps, but more releases requires more resources on maintaining, testing etc
> > so...
> R2.01 could just have all the same of R2.0, with an additional ABI announcement.
> Then nothing needs to be tested.
> 
> - Helin
> 
Then it would be a paper exercise just to bypass an ABI policy, so NACK to that idea.

If (and it's a fairly big if) we do decide we need longer-term maintenance 
branches for maintaining ABI, then we need to do it properly.
This may including doing things liek back-porting relevant (maybe all) features from
later releases that don't break the ABI to the supported version. Bug fixes would
obviously have to be backported.

However, the overhead of this is obvious, since we would now have multiple development
lines to be maintained. 

Regards,
/Bruce
  

Patch

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 {