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

[Zhang, Helin]

> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai at redhat.com]
> Sent: Friday, June 12, 2015 4:15 PM
> To: Zhang, Helin; Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim;
> nhorman at tuxdriver.com
> Cc: dev at 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 at redhat.com]
> >> Sent: Friday, June 12, 2015 3:24 PM
> >> To: Thomas Monjalon; Olivier MATZ; O'Driscoll, Tim; Zhang, Helin;
> >> nhorman at tuxdriver.com
> >> Cc: dev at 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 at 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 -