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

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

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.

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