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

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

Hi Maxime, David, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> On 6/6/23 23:01, Chautru, Nicolas wrote:
> > Hi David,
> >
> >> -----Original Message-----
> >> From: David Marchand <david.marchand@redhat.com>> >>
> >> 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;
> >   };
> 
> The best thing, as suggested by David, would be to move all the ops out of
> struct rte_bbdev, as these should not be visible to the application.

That would be quite disruptive across all PMDs and possible perf impact to validate. I don’t think this is anywhere realistic to consider such a change in 23.11.
I believe moving these function at the end of the structure is a good compromise to avoid future breakage of rte_bbdev structure with almost seamless impact (purely a ABI break when moving into 23.11 which is not avoidable. Retrospectively we should have done that in 22.11 really. 

What do you think Maxime, David? Based on this I can adjust the change for 23.11 and update slightly the deprecation notice accordingly. 

Thanks
Nic

> >>>
> >>> 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 ;
> >>
> >>
> >> --