Re: [dpdk-dev] [PATCH] cryptodev: version rte_cryptodev_info_get function

[Bruce Richardson <bruce.richardson@intel.com>]

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.trahe@intel.com>
> > Sent: Tuesday, April 14, 2020 7:27 PM
> > To: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org
> > Cc: Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@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-bounces@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.kusztal@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:
> > > >
> > > >