[PATCH v1 1/1] doc: announce change in bbdev api related to operation extension
Maxime Coquelin
maxime.coquelin at redhat.com
Thu Jun 8 10:47:20 CEST 2023
On 6/6/23 23:01, Chautru, Nicolas wrote:
> Hi David,
>
>> -----Original Message-----
>> From: David Marchand <david.marchand at redhat.com>
>> Sent: Tuesday, June 6, 2023 2:21 AM
>> To: Chautru, Nicolas <nicolas.chautru at intel.com>
>> Cc: Maxime Coquelin <maxime.coquelin at redhat.com>; Stephen Hemminger
>> <stephen at networkplumber.org>; dev at dpdk.org; Rix, Tom
>> <trix at redhat.com>; hemant.agrawal at nxp.com; Vargas, Hernan
>> <hernan.vargas at 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 at 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
>
More information about the dev
mailing list