On Wed, Apr 15, 2020 at 06:24:19PM +0100, Trahe, Fiona wrote: > Hi Ray/Bruce/Thomas, > Hi Fiona,
answers as best I can give are inline below. > We've been trying to make sense of the versioning scheme - but as the docs > describe and give examples > of a 2-number scheme and the code has a 3-number scheme it's pretty > confusing. > If you can help me fill out the following table this may help: > > VERSION | ABI_VERSION | LIBABIVER| stable soname | lib filename | > new section with ABI break in map file > 19.11.0 | 20.0 | 1 | stable.so.20.0 | > stable.so.20.0.0 | > 20.02.0-rc0| 20.1 | 1 | n/a > 20.02.0 | 20.0.1 | 0.1 | stable.so.20.0 | > stable.so.20.0.1 | > 20.05.0 | 20.0.2 | 0.1 | stable.so.20.0 | > stable.so.20.0.2 | 20.0.2 (or 21? use impending stable > ABI_VERSION?) > 20.08.0 | 20.0.3 | 0.1 | stable.so.20.0 | > stable.so.20.0.3 | > 20.11.0 | 21 | 0.1 | stable.so.21 | > stable.so.21.0 | (Can we move back to a 2 number ABI_VERSION at > this stage? ) > 21.02.0 | 21.1 | 0.1 | stable.so.21 | > stable.so.21.1 | > > Note, first 2 ABI_VERSIONS above were mistakes, soname needs to be same as > 19.11 to avoid ABI breakage, so it was changed to 3-part number, with first 2 > used in soname. > > Questions: > 1. For this year, is major a 2-part num 20.0 (and there will never be a > 20.1,20.2, etc) and minor is the 3rd number? Yes, for this year only. > 2. Can we move back to a 2-number scheme in 20.11? That is the plan, yes. > 3. What uses lib filename? (This is described in config/meson.build) Guessing > stable soname is name on which apps depend, and is a sym link, pointing to > lib filename? In general, yes. > 4. What does LIBABIVER have to do with this ? it was 1, changed to 0.1 in > 20.2 Not sure about this one, what it refers to. > 5a. If in 20.05 we add a version of a fn which breaks ABI 20.0, what should > the name of the original function be? fn_v20, or fn_v20.0 In technical terms it really doesn't matter, it's just a name that will be looked up in a table. I don't think we strictly enforce the naming, so whatever is clearest is best. I'd suggest the former. > Or does it even matter, i.e. is the middle param to this macro arbitrary and > just needs to match the fn name, e.g. if fn name was fn_vxyz could macro be: > VERSION_SYMBOL(fn, _vxyz, 20.0); Yes. > 5b. what should the name of the new fn be? fn_v2002 or fn_v21? Does it need > to correspond to the new section in .map? If v2002, doesn't that imply it's > compatible with 20.0 ABI > So shouldn't the new section be DPDK_21 and the new fn be fn_v21. > 6. Assume we go with DPDK_21 and fn_v21. An app built against 20.05 has a > dependency on fn_v21 (because of the BIND_DEFAULT) but on otherfn_v20.0 of > all other fns? > If fn_v21 is changed again in 20.08, is there any expectation that the app > built against 20.05 must work against the 20.08 fn of the same name? > i.e. is compatibility required on fn changes across minor nonstable releases? > If not then jumping ahead to v21 seems the simplest option. > And the "final" fn_v21 in 20.11 is the one that "sticks" and belongs to the > new ABI which then must remain stable til 21.11 For functions that are part of the stable ABI, each change requires a new version, since there is an expectation that 20.05 builds will also work with 20.08. Regards, /Bruce > > > > -----Original Message----- > > From: Trahe, Fiona <fiona.tr...@intel.com> > > Sent: Tuesday, April 14, 2020 7:27 PM > > To: Ray Kinsella <m...@ashroe.eu>; dev@dpdk.org > > Cc: Trahe, Fiona <fiona.tr...@intel.com>; Kusztal, ArkadiuszX > > <arkadiuszx.kusz...@intel.com> > > Subject: RE: [dpdk-dev] [PATCH] cryptodev: version rte_cryptodev_info_get > > function > > > > Hi Ray, > > > > We're going to need hep to understand the numbering. > > The examples here > > http://doc.dpdk.org/guides/contributing/abi_versioning.html#major-abi-versions > > show only major numbers in the .map file. > > Whereas the map files all have major.minor. > > > > The example shows that to add a new version of an existing fn one should > > use the next major number, > > "When an ABI change is made between major ABI versions to a given library, > > a new section is added to > > that library’s version map describing the impending new ABI version" > > so > > - Old fn becomes fn_v20() > > - VERSION_SYMBOL(fn, _v20, 20); - maps node 20 to fn_v20() for dynamic > > linking > > - New fn becomes fn_v21() > > - BIND_DEFAULT_SYMBOL(fn, _v21, 21); - maps new builds to fn_v21 for > > dynamic linking and makes > > fn_v21 default > > - MAP_STATIC_SYMBOL(fn_proto, fn_v21); - for static linking > > And the map file should have: > > > > DPDK_21 { > > global: > > rte_cryptodev_info_get; > > } DPDK_20; > > > > When the ABI version moves to node 21 in DPDK20.11, the _v20 symbols can be > > removed. > > > > You suggest using DPDK_20.0.2 and _v2002 > > Can you explain why? > > In 20.0.2 - which number is minor? > > And why 3 numbers? > > > > Regards, > > Fiona > > > > > > > -----Original Message----- > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Ray Kinsella > > > Sent: Tuesday, April 14, 2020 2:54 PM > > > To: dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] cryptodev: version rte_cryptodev_info_get > > > function > > > > > > > > > > > > On 18/03/2020 20:41, Arek Kusztal wrote: > > > > This patch adds versioned function rte_cryptodev_info_get. > > > > Node 20.05 function works the same way it was working before. > > > > Node 20.0 function strips capability added in 20.05 release > > > > to prevent some issues with ABI policy. To do that new capability > > > > array is allocated per device and returned to user instead of the > > > > original array passed by PMD. > > > > > > > > Signed-off-by: Arek Kusztal <arkadiuszx.kusz...@intel.com> > > > > --- > > > > lib/librte_cryptodev/rte_cryptodev.c | 97 > > > > +++++++++++++++++++++++++- > > > > lib/librte_cryptodev/rte_cryptodev.h | 12 +++- > > > > lib/librte_cryptodev/rte_cryptodev_version.map | 7 ++ > > > > 3 files changed, 113 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/lib/librte_cryptodev/rte_cryptodev.c > > > > b/lib/librte_cryptodev/rte_cryptodev.c > > > > index 6d1d0e9..2d030ba 100644 > > > > --- a/lib/librte_cryptodev/rte_cryptodev.c > > > > +++ b/lib/librte_cryptodev/rte_cryptodev.c > > > > @@ -41,6 +41,9 @@ > > > > #include "rte_cryptodev.h" > > > > #include "rte_cryptodev_pmd.h" > > > > > > > > +#include <rte_compat.h> > > > > +#include <rte_function_versioning.h> > > > > + > > > > static uint8_t nb_drivers; > > > > > > > > static struct rte_cryptodev rte_crypto_devices[RTE_CRYPTO_MAX_DEVS]; > > > > @@ -56,6 +59,13 @@ static struct rte_cryptodev_global cryptodev_globals > > > > = { > > > > /* spinlock for crypto device callbacks */ > > > > static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER; > > > > > > > > +static const struct rte_cryptodev_capabilities > > > > +cryptodev_undefined_capabilities[] = { > > > > +RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST() > > > > +}; > > > > + > > > > +struct rte_cryptodev_capabilities > > > > *capability_copies[RTE_CRYPTO_MAX_DEVS]; > > > > +uint8_t is_capability_checked[RTE_CRYPTO_MAX_DEVS]; > > > > > > > > /** > > > > * The user application callback description. > > > > @@ -999,6 +1009,13 @@ rte_cryptodev_close(uint8_t dev_id) > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP); > > > > retval = (*dev->dev_ops->dev_close)(dev); > > > > > > > > + > > > > +if (capability_copies[dev_id]) { > > > > +free(capability_copies[dev_id]); > > > > +capability_copies[dev_id] = NULL; > > > > +} > > > > +is_capability_checked[dev_id] = 0; > > > > + > > > > if (retval < 0) > > > > return retval; > > > > > > > > @@ -1111,11 +1128,12 @@ rte_cryptodev_stats_reset(uint8_t dev_id) > > > > (*dev->dev_ops->stats_reset)(dev); > > > > } > > > > > > > > - > > > > void > > > > -rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info) > > > > +rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info) > > > > { > > > > struct rte_cryptodev *dev; > > > > +const struct rte_cryptodev_capabilities *capability; > > > > +uint8_t counter = 0; > > > > > > > > if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { > > > > CDEV_LOG_ERR("Invalid dev_id=%d", dev_id); > > > > @@ -1129,10 +1147,85 @@ rte_cryptodev_info_get(uint8_t dev_id, struct > > > > rte_cryptodev_info > > > *dev_info) > > > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > > > > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > > > > > > > > +if (capability_copies[dev_id] == NULL) { > > > > +if (!is_capability_checked[dev_id]) { > > > > +uint8_t found_invalid_capa = 0; > > > > + > > > > +for (capability = dev_info->capabilities; > > > > +capability->op != RTE_CRYPTO_OP_TYPE_UNDEFINED; > > > > +++capability, ++counter) { > > > > +if (capability->op == RTE_CRYPTO_OP_TYPE_SYMMETRIC && > > > > +capability->sym.xform_type == > > > > +RTE_CRYPTO_SYM_XFORM_AEAD > > > > +&& capability->sym.aead.algo >= > > > > +RTE_CRYPTO_AEAD_CHACHA20_POLY1305) { > > > > +found_invalid_capa = 1; > > > > +counter--; > > > > +} > > > > +} > > > > +is_capability_checked[dev_id] = 1; > > > > +if (found_invalid_capa) { > > > > +capability_copies[dev_id] = malloc(counter * > > > > +sizeof(struct rte_cryptodev_capabilities)); > > > > +if (capability_copies[dev_id] == NULL) { > > > > + /* > > > > + * error case - no memory to store the trimmed list, > > > > + * so have to return an empty list > > > > + */ > > > > +dev_info->capabilities = > > > > +cryptodev_undefined_capabilities; > > > > +is_capability_checked[dev_id] = 0; > > > > +} else { > > > > +counter = 0; > > > > +for (capability = dev_info->capabilities; > > > > +capability->op != > > > > +RTE_CRYPTO_OP_TYPE_UNDEFINED; > > > > +capability++) { > > > > +if (!(capability->op == > > > > +RTE_CRYPTO_OP_TYPE_SYMMETRIC > > > > +&& capability->sym.xform_type == > > > > +RTE_CRYPTO_SYM_XFORM_AEAD > > > > +&& capability->sym.aead.algo >= > > > > + > > > RTE_CRYPTO_AEAD_CHACHA20_POLY1305)) { > > > > +capability_copies[dev_id][counter++] = > > > > +*capability; > > > > +} > > > > +} > > > > +dev_info->capabilities = > > > > +capability_copies[dev_id]; > > > > +} > > > > +} > > > > +} > > > > +} else > > > > +dev_info->capabilities = capability_copies[dev_id]; > > > > + > > > > dev_info->driver_name = dev->device->driver->name; > > > > dev_info->device = dev->device; > > > > } > > > > +VERSION_SYMBOL(rte_cryptodev_info_get, _v20, 20.0); > > > > + > > > > +void > > > > +rte_cryptodev_info_get_v2005(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info) > > > > > > should be rte_cryptodev_info_get_v2002 > > > > > > > +{ > > > > +struct rte_cryptodev *dev; > > > > > > > > +if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { > > > > +CDEV_LOG_ERR("Invalid dev_id=%d", dev_id); > > > > +return; > > > > +} > > > > + > > > > +dev = &rte_crypto_devices[dev_id]; > > > > + > > > > +memset(dev_info, 0, sizeof(struct rte_cryptodev_info)); > > > > + > > > > +RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > > > > +(*dev->dev_ops->dev_infos_get)(dev, dev_info); > > > > + > > > > +dev_info->driver_name = dev->device->driver->name; > > > > +dev_info->device = dev->device; > > > > +} > > > > +MAP_STATIC_SYMBOL(void rte_cryptodev_info_get(uint8_t dev_id, > > > > +struct rte_cryptodev_info *dev_info), rte_cryptodev_info_get_v2005); > > > > > > should be rte_cryptodev_info_get_v2002 > > > > > > > > > > > int > > > > rte_cryptodev_callback_register(uint8_t dev_id, > > > > diff --git a/lib/librte_cryptodev/rte_cryptodev.h > > > > b/lib/librte_cryptodev/rte_cryptodev.h > > > > index 437b8a9..06ce2f2 100644 > > > > --- a/lib/librte_cryptodev/rte_cryptodev.h > > > > +++ b/lib/librte_cryptodev/rte_cryptodev.h > > > > @@ -24,6 +24,9 @@ extern "C" { > > > > #include <rte_common.h> > > > > #include <rte_config.h> > > > > > > > > +#include <rte_compat.h> > > > > +#include <rte_function_versioning.h> > > > > + > > > > extern const char **rte_cyptodev_names; > > > > > > > > /* Logging Macros */ > > > > @@ -758,7 +761,14 @@ rte_cryptodev_stats_reset(uint8_t dev_id); > > > > * the last valid element has it's op field set to > > > > * RTE_CRYPTO_OP_TYPE_UNDEFINED. > > > > */ > > > > -extern void > > > > +void > > > > +rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info); > > > > + > > > > +void > > > > +rte_cryptodev_info_get_v2005(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info); > > > > +BIND_DEFAULT_SYMBOL(rte_cryptodev_info_get, _v2005, 20.05); > > > > + > > > > +void > > > > rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info > > > > *dev_info); > > > do we still need this forward declaration? > > > > > > > > > > > > > > > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map > > > b/lib/librte_cryptodev/rte_cryptodev_version.map > > > > index 6e41b4b..d6c945a 100644 > > > > --- a/lib/librte_cryptodev/rte_cryptodev_version.map > > > > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map > > > > @@ -58,6 +58,13 @@ DPDK_20.0 { > > > > local: *; > > > > }; > > > > > > > > + > > > > +DPDK_20.05 { > > > > > > should be DPDK_20.0.2 > > > > > > > +global: > > > > +rte_cryptodev_info_get; > > > > +} DPDK_20.0; > > > > + > > > > + > > > > EXPERIMENTAL { > > > > global: > > > > > > > >
