RE: [PATCH v1 1/1] doc: announce change in bbdev api related to operation extension

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

Hi David, 

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, June 6, 2023 2:21 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dev@dpdk.org; Rix, Tom
> <trix@redhat.com>; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>
> Subject: Re: [PATCH v1 1/1] doc: announce change in bbdev api related to
> operation extension
> 
> On Mon, Jun 5, 2023 at 10:08 PM Chautru, Nicolas
> <nicolas.chautru@intel.com> wrote:
> > Wrt the MLD functions: these are new into the related serie but still the
> break the ABI since the struct rte_bbdev includes these functions hence
> causing offset changes.
> >
> > Should I then just rephrase as:
> >
> > +* bbdev: Will extend the API to support the new operation type
> >  +``RTE_BBDEV_OP_MLDTS`` as per
> >  +  this `v1
> >  +<https://patches.dpdk.org/project/dpdk/list/?series=28192>`. This  +
> > will notably introduce  +  new symbols for
> > ``rte_bbdev_dequeue_mldts_ops``,  +``rte_bbdev_enqueue_mldts_ops``
> > into the stuct rte_bbdev.
> 
> I don't think we need this deprecation notice.
> 
> 
> Do you need to expose those new mldts ops in rte_bbdev?
> Can't they go to dev_ops?
> If you can't, at least moving those new ops at the end of the structure
> would avoid the breakage on rte_bbdev.

It would probably be best to move all these ops at the end of the structure (ie. keep them together). 
In that case the deprecation notice would call out that the rte_bbdev structure content is more generally modified. Probably best for the longer run. 
David, Maxime, ok with that option?

struct __rte_cache_aligned rte_bbdev {
     rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops;
     rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops;
     rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops;
     rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops;
     rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops;
     rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops;
     rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops;
     rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops;
     rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops;
     rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops;
     const struct rte_bbdev_ops *dev_ops;  
     struct rte_bbdev_data *data;  
     enum rte_bbdev_state state;  
     struct rte_device *device; 
     struct rte_bbdev_cb_list list_cbs;
     struct rte_intr_handle *intr_handle; 
 };



> 
> 
> >
> > Pasting below the ABI results for reference
> >
> >   [C] 'function rte_bbdev* rte_bbdev_allocate(const char*)' at
> rte_bbdev.c:174:1 has some indirect sub-type changes:
> >     return type changed:
> >       in pointed to type 'struct rte_bbdev' at rte_bbdev.h:498:1:
> >         type size hasn't changed
> >         2 data member insertions:
> >           'rte_bbdev_enqueue_mldts_ops_t rte_bbdev::enqueue_mldts_ops',
> at offset 640 (in bits) at rte_bbdev.h:520:1
> >           'rte_bbdev_dequeue_mldts_ops_t rte_bbdev::dequeue_mldts_ops',
> at offset 704 (in bits) at rte_bbdev.h:522:1
> >         7 data member changes (9 filtered):
> >           type of 'rte_bbdev_dequeue_fft_ops_t rte_bbdev::dequeue_fft_ops'
> changed:
> >             underlying type 'typedef uint16_t (rte_bbdev_queue_data*,
> rte_bbdev_fft_op**, typedef uint16_t)*' changed:
> >               in pointed to type 'function type typedef uint16_t
> (rte_bbdev_queue_data*, rte_bbdev_fft_op**, typedef uint16_t)':
> >                 parameter 2 of type 'rte_bbdev_fft_op**' has sub-type changes:
> >                   in pointed to type 'rte_bbdev_fft_op*':
> >                     in pointed to type 'struct rte_bbdev_fft_op' at
> rte_bbdev_op.h:978:1:
> >                       type size changed from 832 to 1664 (in bits)
> >                       1 data member change:
> >                         type of 'rte_bbdev_op_fft rte_bbdev_fft_op::fft' changed:
> >                           type size changed from 640 to 1472 (in bits)
> >                           6 data member insertions:
> >                             'rte_bbdev_op_data
> rte_bbdev_op_fft::dewindowing_input', at offset 256 (in bits) at
> rte_bbdev_op.h:771:1
> >                             'int8_t rte_bbdev_op_fft::freq_resample_mode', at offset
> 768 (in bits) at rte_bbdev_op.h:807:1
> >                             'uint16_t rte_bbdev_op_fft::output_depadded_size', at
> offset 784 (in bits) at rte_bbdev_op.h:809:1
> >                             'uint16_t rte_bbdev_op_fft::cs_theta_0[12]', at offset 800
> (in bits) at rte_bbdev_op.h:811:1
> >                             'uint32_t rte_bbdev_op_fft::cs_theta_d[12]', at offset 992
> (in bits) at rte_bbdev_op.h:813:1
> >                             'int8_t rte_bbdev_op_fft::time_offset[12]', at offset 1376
> (in bits) at rte_bbdev_op.h:815:1
> >                           17 data member changes:
> >                             'rte_bbdev_op_data
> rte_bbdev_op_fft::power_meas_output' offset changed from 256 to 384 (in
> bits) (by +128 bits)
> >                             'uint32_t rte_bbdev_op_fft::op_flags' offset changed from
> 384 to 512 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::input_sequence_size' offset
> changed from 416 to 544 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::input_leading_padding'
> offset changed from 432 to 560 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::output_sequence_size' offset
> changed from 448 to 576 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::output_leading_depadding'
> offset changed from 464 to 592 (in bits) (by +128 bits)
> >                             'uint8_t rte_bbdev_op_fft::window_index[6]' offset
> changed from 480 to 608 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::cs_bitmap' offset changed
> from 528 to 656 (in bits) (by +128 bits)
> >                             'uint8_t rte_bbdev_op_fft::num_antennas_log2' offset
> changed from 544 to 672 (in bits) (by +128 bits)
> >                             'uint8_t rte_bbdev_op_fft::idft_log2' offset changed from
> 552 to 680 (in bits) (by +128 bits)
> >                             'uint8_t rte_bbdev_op_fft::dft_log2' offset changed from
> 560 to 688 (in bits) (by +128 bits)
> >                             'int8_t rte_bbdev_op_fft::cs_time_adjustment' offset
> changed from 568 to 696 (in bits) (by +128 bits)
> >                             'int8_t rte_bbdev_op_fft::idft_shift' offset changed from
> 576 to 704 (in bits) (by +128 bits)
> >                             'int8_t rte_bbdev_op_fft::dft_shift' offset changed from
> 584 to 712 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::ncs_reciprocal' offset
> changed from 592 to 720 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::power_shift' offset changed
> from 608 to 736 (in bits) (by +128 bits)
> >                             'uint16_t rte_bbdev_op_fft::fp16_exp_adjust' offset
> changed from 624 to 752 (in bits) (by +128 bits)
> >           'const rte_bbdev_ops* rte_bbdev::dev_ops' offset changed from 640
> to 768 (in bits) (by +128 bits)
> >           'rte_bbdev_data* rte_bbdev::data' offset changed from 704 to 832
> (in bits) (by +128 bits)
> >           'rte_bbdev_state rte_bbdev::state' offset changed from 768 to 896
> (in bits) (by +128 bits)
> >           'rte_device* rte_bbdev::device' offset changed from 832 to 960 (in
> bits) (by +128 bits)
> >           'rte_bbdev_cb_list rte_bbdev::list_cbs' offset changed from 896 to
> 1024 (in bits) (by +128 bits)
> >           'rte_intr_handle* rte_bbdev::intr_handle' offset changed
> > from 1024 to 1152 (in bits) (by +128 bits)
> 
> As for the report on the rte_bbdev_op_fft structure changes:
> - wrt to its size, I think it is okay to waive it, rte_bbdev_fft_op objects are
> coming from a bbdev mempool which is created by the bbdev library itself
> (with the right element size if the application asked for RTE_BBDEV_OP_FFT
> type),
> - wrt to the fields locations, an application may have been touching those
> fields, so moving all the added fields at the end of the structure would be
> better.
>   But on the other hand, an application will have to call an fft_ops
> experimental API at some point, and the application developer is already
> warned that ABI is not preserved on this part of the API,
> 
> So I would waive the changes on rte_bbdev_fft_op with something like:
> 
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore index
> 3ff51509de..3cdce69418 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -36,6 +36,8 @@
>  [suppress_type]
>          type_kind = enum
>          changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM,
> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> +[suppress_type]
> +       name = rte_bbdev_fft_op


OK I did not know about this method. Shouldn't this apply more generally to all experimental structures?
This can be added into the serie for 23.11.


Thanks
Nic



> 
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ; Temporary exceptions till next major ABI version ;
> 
> 
> --
> David Marchand