Re: [RFC PATCH 2/7] telemetry: add uint type as alias for u64

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

On Thu, Dec 15, 2022 at 02:36:51PM +0100, Thomas Monjalon wrote:
> 15/12/2022 10:44, Bruce Richardson:
> > On Wed, Dec 14, 2022 at 09:38:45AM -0800, Tyler Retzlaff wrote:
> > > On Tue, Dec 13, 2022 at 06:27:25PM +0000, Bruce Richardson wrote:
> > > > For future standardization on the "uint" name for unsigned values rather
> > > > than the existing "u64" one, we can for now:
> > > > * rename all internal values to use uint rather than u64
> > > > * add new function names to alias the existing u64 ones
> > > > 
> > > > Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---
> > > >  lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++++++++++++++++++++++
> > > >  lib/telemetry/telemetry.c      | 16 +++++++--------
> > > >  lib/telemetry/telemetry_data.c | 28 ++++++++++++++++++--------
> > > >  lib/telemetry/telemetry_data.h |  4 ++--
> > > >  lib/telemetry/version.map      |  7 +++++++
> > > >  5 files changed, 73 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > > > index c2ad65effe..60877dae0a 100644
> > > > --- a/lib/telemetry/rte_telemetry.h
> > > > +++ b/lib/telemetry/rte_telemetry.h
> > > > @@ -8,6 +8,8 @@
> > > >  #ifndef _RTE_TELEMETRY_H_
> > > >  #define _RTE_TELEMETRY_H_
> > > >  
> > > > +#include <rte_compat.h>
> > > > +
> > > >  #ifdef __cplusplus
> > > >  extern "C" {
> > > >  #endif
> > > > @@ -121,6 +123,22 @@ int
> > > >  rte_tel_data_add_array_int(struct rte_tel_data *d, int x);
> > > >  
> > > >  /**
> > > 
> > > when adding __rte_experimental api i have been asked to add the
> > > following boilerplate documentation block. i'm not pushing it, just
> > > recalling it is what i get asked for, so in case it's something we do?
> > > see lib/eal/include/rte_thread.h as an example
> > > 
> > > 
> > > ```
> > >  * @warning
> > >  * @b EXPERIMENTAL: this API may change without prior notice.
> > > ```
> > >
> > 
> > Ok, thanks for the notice.
> > 
> > Actually, related to this, since we are adding these functions as aliases
> > for existing stable functions, I would like to see these being added not as
> > experimental. The reason for that, is that while they are experimental, we
> > cannot feasibly mark the old function names as deprecated. :-(
> > 
> > Adding Thomas and David on CC for their thoughts.
> 
> Is it related to telemetry?
> 
> In general, yes we cannot deprecate something if there is no stable replacement.
> The recommended step is to introduce a new experimental API
> and deprecate the old one when the new API is stable.
> 
Yes, understood.
What we are really trying to do here is to rename an API, by process of
adding the new API and then marking the old one as deprecated. The small
issue is that adding the new one it is by default experimental, meaning we
need to wait for deprecating old one. Ideally, as soon as the new API is
added, we would like to point people to use that, but can't really do so
while it is experimental.

---

By way of explicit detail, Morten pointed out the inconsistency in the
telemetry APIs and types:

* we have add_*_int, which takes a 32-bit signed value
* we have add_*_u64 which takes 64-bit unsigned (as name suggests).

The ideal end-state is to always use 64-bit values (since there is no space
saving from 32-bit as a union is used), and just name everything as "int"
or "uint" for signed/unsigned. The two big steps here are:

* expanding type of the "int" functions to take 64-bit parameters - this is
  ABI change but not API one, since existing code will happily promote
  values on compile. Therefore, we just use ABI versioning to have a 32-bit
  version for older linked binaries.
* the rename of the rte_tel_data_add_array_u64 and
  rte_tel_data_add_dict_u64 to *_uint variants. Though keeping
  compatibility is easier, as we can just add new functions, the overall
  process is slower since the new functions technically should be added as
  experimental - hence the email. For the case of function renaming, do we
  still need to have the "renamed" versions as experimental initially?

Given where we are in the overall DPDK releases cycle, it's not a major
issue either way, I just would like some clarity. My main concern with
having it spread over a couple of releases, is that it's more likely a step
will be missed/forgotten somewhere along the way! 

/Bruce