Hi Maxime, David, > -----Original Message----- > From: Maxime Coquelin <maxime.coque...@redhat.com> > > On 6/6/23 23:01, Chautru, Nicolas wrote: > > Hi David, > > > >> -----Original Message----- > >> From: David Marchand <david.march...@redhat.com>> >> > >> On Mon, Jun 5, 2023 at 10:08 PM Chautru, Nicolas > >> <nicolas.chau...@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 ; > >> > >> > >> --
