Hi Tim, Helin,

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

quoted

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

quoted

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