[v1,1/1] doc: announce change in bbdev api related to operation extension
Checks
Commit Message
Intent to extend the bbdev api in DPDK 23.11 for new optional
operations as well as addition of new members in existing
structure.
Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
doc/guides/rel_notes/deprecation.rst | 6 ++++++
1 file changed, 6 insertions(+)
Comments
On Fri, 26 May 2023 02:11:32 +0000
Nicolas Chautru <nicolas.chautru@intel.com> wrote:
> +
> +* 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 also introduce
> + new symbols for ``rte_bbdev_dequeue_mldts_ops``, ``rte_bbdev_enqueue_mldts_ops``,
> + ``rte_bbdev_mldts_op_alloc_bulk`` and ``rte_bbdev_mldts_op_free_bulk``. This will also extend
> + the API related to the FFT operation in ``rte_bbdev_op_fft``.
> --
New symbols do not require a deprecation notice.
Only changes and removal.
On 5/26/23 05:47, Stephen Hemminger wrote:
> On Fri, 26 May 2023 02:11:32 +0000
> Nicolas Chautru <nicolas.chautru@intel.com> wrote:
>
>> +
>> +* 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 also introduce
>> + new symbols for ``rte_bbdev_dequeue_mldts_ops``, ``rte_bbdev_enqueue_mldts_ops``,
>> + ``rte_bbdev_mldts_op_alloc_bulk`` and ``rte_bbdev_mldts_op_free_bulk``. This will also extend
>> + the API related to the FFT operation in ``rte_bbdev_op_fft``.
>> --
>
> New symbols do not require a deprecation notice.
> Only changes and removal.
>
I agree with Stephen.
There is some changes in struct rte_bbdev_op_fft, but the related API
are experimental, so I think it is not needed to have a deprecation
notice.
Regards,
Maxime
Hi Maxime,
So basically the fft structure change is okay since these are still marked as rte_experimental (it got reported in the ABI report though).
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.
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)
Thanks
Nic
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, June 5, 2023 12:08 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Chautru, Nicolas
> <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; Rix, Tom <trix@redhat.com>; hemant.agrawal@nxp.com;
> david.marchand@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> Subject: Re: [PATCH v1 1/1] doc: announce change in bbdev api related to
> operation extension
>
>
>
> On 5/26/23 05:47, Stephen Hemminger wrote:
> > On Fri, 26 May 2023 02:11:32 +0000
> > Nicolas Chautru <nicolas.chautru@intel.com> wrote:
> >
> >> +
> >> +* 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 also introduce
> >> + new symbols for ``rte_bbdev_dequeue_mldts_ops``,
> >> +``rte_bbdev_enqueue_mldts_ops``,
> >> + ``rte_bbdev_mldts_op_alloc_bulk`` and
> >> +``rte_bbdev_mldts_op_free_bulk``. This will also extend
> >> + the API related to the FFT operation in ``rte_bbdev_op_fft``.
> >> --
> >
> > New symbols do not require a deprecation notice.
> > Only changes and removal.
> >
> I agree with Stephen.
> There is some changes in struct rte_bbdev_op_fft, but the related API are
> experimental, so I think it is not needed to have a deprecation notice.
>
> Regards,
> Maxime
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.
>
> 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
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Temporary exceptions till next major ABI version ;
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
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
>
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 ;
> >>
> >>
> >> --
On 6/12/23 22:53, Chautru, Nicolas wrote:
> 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.
If we are going to break the ABI, better to do the right rework
directly. Otherwise we'll end-up breaking it again next year.
IMHO, moving these ops should be quite trivial and not much work.
Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it may
not break the ABI, but that's a bit fragile:
- rte_bbdev_devices[] is not static, but is placed in the BSS section so
should be OK
- struct rte_bbdev is cache-aligned, so it may work if adding these two
ops do not overlap a cacheline which depends on the CPU architecture.
Maxime
> 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
>
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> On 6/12/23 22:53, Chautru, Nicolas wrote:
> > 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.
>
> If we are going to break the ABI, better to do the right rework directly. Otherwise
> we'll end-up breaking it again next year.
With the suggested change, this will not break ABI next year. Any future functions are added at the end of the structure anyway.
>
> IMHO, moving these ops should be quite trivial and not much work.
>
> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it may not
> break the ABI, but that's a bit fragile:
> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
> should be OK
> - struct rte_bbdev is cache-aligned, so it may work if adding these two
> ops do not overlap a cacheline which depends on the CPU architecture.
If you prefer to add the only 2 new functions at the end of the structure that is okay. I believe it would be cleaner to move all these enqueue/dequeue funs down together without drawback I can think of. Let me know.
>
> Maxime
>
> > 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
> >
Hi,
On 6/13/23 19:16, Chautru, Nicolas wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
>>
>> On 6/12/23 22:53, Chautru, Nicolas wrote:
>>> 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.
>>
>> If we are going to break the ABI, better to do the right rework directly. Otherwise
>> we'll end-up breaking it again next year.
>
> With the suggested change, this will not break ABI next year. Any future functions are added at the end of the structure anyway.
I'm not so sure, it depends if adding a new field at the end cross a
cacheline boundary or not:
/*
* Global array of all devices. This is not static because it's used by the
* inline enqueue and dequeue functions
*/
struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
If the older inlined functions used by the application retrieve the dev
pointer from the array directly (they do) and added new fields in new
version cross a cacheline, then there will be a misalignement between
the new lib version and the application using the older inlined
functions.
ABI-wise, this is not really future proof.
>
>>
>> IMHO, moving these ops should be quite trivial and not much work.
>>
>> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
>> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it may not
>> break the ABI, but that's a bit fragile:
>> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
>> should be OK
>> - struct rte_bbdev is cache-aligned, so it may work if adding these two
>> ops do not overlap a cacheline which depends on the CPU architecture.
>
> If you prefer to add the only 2 new functions at the end of the structure that is okay. I believe it would be cleaner to move all these enqueue/dequeue funs down together without drawback I can think of. Let me know.
Adding the new ones at the end is not future proof, but at least it does
not break ABI just for cosmetic reasons (that's a big drawback IMHO).
I just checked using pahole:
struct rte_bbdev {
rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8 */
rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8 */
rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8 */
rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
const struct rte_bbdev_ops * dev_ops; /* 80 8 */
struct rte_bbdev_data * data; /* 88 8 */
enum rte_bbdev_state state; /* 96 4 */
/* XXX 4 bytes hole, try to pack */
struct rte_device * device; /* 104 8 */
struct rte_bbdev_cb_list list_cbs; /* 112 16 */
/* --- cacheline 2 boundary (128 bytes) --- */
struct rte_intr_handle * intr_handle; /* 128 8 */
/* size: 192, cachelines: 3, members: 16 */
/* sum members: 132, holes: 1, sum holes: 4 */
/* padding: 56 */
} __attribute__((__aligned__(64)));
We're lucky on x86, we still have 56 bytes, so we can add 7 new ops at
the end before breaking the ABI if I'm not mistaken.
I checked the other architecture, and it seems we don't support any with
32B cacheline size so we're good for a while.
Maxime
>
>>
>> Maxime
>>
>>> 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
>>>
>
On Tue, 13 Jun 2023 22:00:25 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >>
> >> If we are going to break the ABI, better to do the right rework directly. Otherwise
> >> we'll end-up breaking it again next year.
> >
> > With the suggested change, this will not break ABI next year. Any future functions are added at the end of the structure anyway.
Do it right in 23.11, break the ABI and fix the few drivers.
It is not hard to have one ops struct (and it can/should be const) that is pointed
to by the bbdev. That will hide the ops from the application.
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Hi,
>
> On 6/13/23 19:16, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> >>
> >> On 6/12/23 22:53, Chautru, Nicolas wrote:
> >>> 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.
> >>
> >> If we are going to break the ABI, better to do the right rework
> >> directly. Otherwise we'll end-up breaking it again next year.
> >
> > With the suggested change, this will not break ABI next year. Any future
> functions are added at the end of the structure anyway.
>
> I'm not so sure, it depends if adding a new field at the end cross a cacheline
> boundary or not:
>
> /*
> * Global array of all devices. This is not static because it's used by the
> * inline enqueue and dequeue functions
> */
> struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
>
> If the older inlined functions used by the application retrieve the dev pointer
> from the array directly (they do) and added new fields in new version cross
> a cacheline, then there will be a misalignement between the new lib version
> and the application using the older inlined functions.
>
> ABI-wise, this is not really future proof.
>
> >
> >>
> >> IMHO, moving these ops should be quite trivial and not much work.
> >>
> >> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
> >> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it may
> >> not break the ABI, but that's a bit fragile:
> >> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
> >> should be OK
> >> - struct rte_bbdev is cache-aligned, so it may work if adding these two
> >> ops do not overlap a cacheline which depends on the CPU architecture.
> >
> > If you prefer to add the only 2 new functions at the end of the structure
> that is okay. I believe it would be cleaner to move all these
> enqueue/dequeue funs down together without drawback I can think of. Let
> me know.
>
> Adding the new ones at the end is not future proof, but at least it does not
> break ABI just for cosmetic reasons (that's a big drawback IMHO).
>
> I just checked using pahole:
>
> struct rte_bbdev {
> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8
> */
> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8
> */
> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8
> */
> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8
> */
> /* --- cacheline 1 boundary (64 bytes) --- */
> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
> const struct rte_bbdev_ops * dev_ops; /* 80 8 */
> struct rte_bbdev_data * data; /* 88 8 */
> enum rte_bbdev_state state; /* 96 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct rte_device * device; /* 104 8 */
> struct rte_bbdev_cb_list list_cbs; /* 112 16 */
> /* --- cacheline 2 boundary (128 bytes) --- */
> struct rte_intr_handle * intr_handle; /* 128 8 */
>
> /* size: 192, cachelines: 3, members: 16 */
> /* sum members: 132, holes: 1, sum holes: 4 */
> /* padding: 56 */
> } __attribute__((__aligned__(64)));
>
> We're lucky on x86, we still have 56 bytes, so we can add 7 new ops at the
> end before breaking the ABI if I'm not mistaken.
>
> I checked the other architecture, and it seems we don't support any with
> 32B cacheline size so we're good for a while.
OK then just adding the new functions at the end, no other cosmetic changes. Will update the patch to match this.
In term of deprecation notice, you are okay with latest draft?
+* 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.
>
> Maxime
>
> >
> >>
> >> Maxime
> >>
> >>> 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
> >>>
> >
On 6/14/23 20:18, Chautru, Nicolas wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Hi,
>>
>> On 6/13/23 19:16, Chautru, Nicolas wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>>>
>>>> On 6/12/23 22:53, Chautru, Nicolas wrote:
>>>>> 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.
>>>>
>>>> If we are going to break the ABI, better to do the right rework
>>>> directly. Otherwise we'll end-up breaking it again next year.
>>>
>>> With the suggested change, this will not break ABI next year. Any future
>> functions are added at the end of the structure anyway.
>>
>> I'm not so sure, it depends if adding a new field at the end cross a cacheline
>> boundary or not:
>>
>> /*
>> * Global array of all devices. This is not static because it's used by the
>> * inline enqueue and dequeue functions
>> */
>> struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
>>
>> If the older inlined functions used by the application retrieve the dev pointer
>> from the array directly (they do) and added new fields in new version cross
>> a cacheline, then there will be a misalignement between the new lib version
>> and the application using the older inlined functions.
>>
>> ABI-wise, this is not really future proof.
>>
>>>
>>>>
>>>> IMHO, moving these ops should be quite trivial and not much work.
>>>>
>>>> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
>>>> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it may
>>>> not break the ABI, but that's a bit fragile:
>>>> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
>>>> should be OK
>>>> - struct rte_bbdev is cache-aligned, so it may work if adding these two
>>>> ops do not overlap a cacheline which depends on the CPU architecture.
>>>
>>> If you prefer to add the only 2 new functions at the end of the structure
>> that is okay. I believe it would be cleaner to move all these
>> enqueue/dequeue funs down together without drawback I can think of. Let
>> me know.
>>
>> Adding the new ones at the end is not future proof, but at least it does not
>> break ABI just for cosmetic reasons (that's a big drawback IMHO).
>>
>> I just checked using pahole:
>>
>> struct rte_bbdev {
>> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
>> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
>> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
>> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
>> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8
>> */
>> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8
>> */
>> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8
>> */
>> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8
>> */
>> /* --- cacheline 1 boundary (64 bytes) --- */
>> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
>> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
>> const struct rte_bbdev_ops * dev_ops; /* 80 8 */
>> struct rte_bbdev_data * data; /* 88 8 */
>> enum rte_bbdev_state state; /* 96 4 */
>>
>> /* XXX 4 bytes hole, try to pack */
>>
>> struct rte_device * device; /* 104 8 */
>> struct rte_bbdev_cb_list list_cbs; /* 112 16 */
>> /* --- cacheline 2 boundary (128 bytes) --- */
>> struct rte_intr_handle * intr_handle; /* 128 8 */
>>
>> /* size: 192, cachelines: 3, members: 16 */
>> /* sum members: 132, holes: 1, sum holes: 4 */
>> /* padding: 56 */
>> } __attribute__((__aligned__(64)));
>>
>> We're lucky on x86, we still have 56 bytes, so we can add 7 new ops at the
>> end before breaking the ABI if I'm not mistaken.
>>
>> I checked the other architecture, and it seems we don't support any with
>> 32B cacheline size so we're good for a while.
>
> OK then just adding the new functions at the end, no other cosmetic changes. Will update the patch to match this.
> In term of deprecation notice, you are okay with latest draft?
>
> +* 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.
This is not needed in the deprecation notice.
If you are willing to announce it, it could be part of the Intel
roadmap.
To be on the safe side, could you try to dynamically link an application
with a DPDK version before this change, then rebuild DPDK with adding
these two fields. Then test with at least 2 devices with test-bbdev and
see if it does not crash or fail?
Thanks,
Maxime
>
>>
>> Maxime
>>
>>>
>>>>
>>>> Maxime
>>>>
>>>>> 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
>>>>>
>>>
>
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> On 6/14/23 20:18, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com> Hi,
> >>
> >> On 6/13/23 19:16, Chautru, Nicolas wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>
> >>>>
> >>>> On 6/12/23 22:53, Chautru, Nicolas wrote:
> >>>>> 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.
> >>>>
> >>>> If we are going to break the ABI, better to do the right rework
> >>>> directly. Otherwise we'll end-up breaking it again next year.
> >>>
> >>> With the suggested change, this will not break ABI next year. Any
> >>> future
> >> functions are added at the end of the structure anyway.
> >>
> >> I'm not so sure, it depends if adding a new field at the end cross a
> >> cacheline boundary or not:
> >>
> >> /*
> >> * Global array of all devices. This is not static because it's used by the
> >> * inline enqueue and dequeue functions
> >> */
> >> struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
> >>
> >> If the older inlined functions used by the application retrieve the
> >> dev pointer from the array directly (they do) and added new fields in
> >> new version cross a cacheline, then there will be a misalignement
> >> between the new lib version and the application using the older inlined
> functions.
> >>
> >> ABI-wise, this is not really future proof.
> >>
> >>>
> >>>>
> >>>> IMHO, moving these ops should be quite trivial and not much work.
> >>>>
> >>>> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
> >>>> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it
> >>>> may not break the ABI, but that's a bit fragile:
> >>>> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
> >>>> should be OK
> >>>> - struct rte_bbdev is cache-aligned, so it may work if adding these two
> >>>> ops do not overlap a cacheline which depends on the CPU
> architecture.
> >>>
> >>> If you prefer to add the only 2 new functions at the end of the
> >>> structure
> >> that is okay. I believe it would be cleaner to move all these
> >> enqueue/dequeue funs down together without drawback I can think of.
> >> Let me know.
> >>
> >> Adding the new ones at the end is not future proof, but at least it
> >> does not break ABI just for cosmetic reasons (that's a big drawback
> IMHO).
> >>
> >> I just checked using pahole:
> >>
> >> struct rte_bbdev {
> >> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
> >> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
> >> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
> >> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
> >> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8
> >> */
> >> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8
> >> */
> >> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8
> >> */
> >> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8
> >> */
> >> /* --- cacheline 1 boundary (64 bytes) --- */
> >> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
> >> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
> >> const struct rte_bbdev_ops * dev_ops; /* 80 8 */
> >> struct rte_bbdev_data * data; /* 88 8 */
> >> enum rte_bbdev_state state; /* 96 4 */
> >>
> >> /* XXX 4 bytes hole, try to pack */
> >>
> >> struct rte_device * device; /* 104 8 */
> >> struct rte_bbdev_cb_list list_cbs; /* 112 16 */
> >> /* --- cacheline 2 boundary (128 bytes) --- */
> >> struct rte_intr_handle * intr_handle; /* 128 8 */
> >>
> >> /* size: 192, cachelines: 3, members: 16 */
> >> /* sum members: 132, holes: 1, sum holes: 4 */
> >> /* padding: 56 */
> >> } __attribute__((__aligned__(64)));
> >>
> >> We're lucky on x86, we still have 56 bytes, so we can add 7 new ops
> >> at the end before breaking the ABI if I'm not mistaken.
> >>
> >> I checked the other architecture, and it seems we don't support any
> >> with 32B cacheline size so we're good for a while.
> >
> > OK then just adding the new functions at the end, no other cosmetic
> changes. Will update the patch to match this.
> > In term of deprecation notice, you are okay with latest draft?
> >
> > +* 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.
>
> This is not needed in the deprecation notice.
> If you are willing to announce it, it could be part of the Intel roadmap.
>
I still see this abi failure as we extend the struct (see below), what is the harm in calling it out in the deprecation notice?
1 function with some indirect sub-type change:
[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 enqueue_mldts_ops', at offset 1088 (in bits) at rte_bbdev.h:527:1
'rte_bbdev_dequeue_mldts_ops_t dequeue_mldts_ops', at offset 1152 (in bits) at rte_bbdev.h:529:1
no data member changes (12 filtered);
Error: ABI issue reported for abidiff --suppr /home-local/jenkins-local/jenkins-agent/workspace/Generic-DPDK-Compile-ABI@2/dpdk/devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 build_install/usr/local/include reference/usr/local/lib64/librte_bbdev.so.23.0 build_install/usr/local/lib64/librte_bbdev.so.23.2
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).
>
> To be on the safe side, could you try to dynamically link an application with
> a DPDK version before this change, then rebuild DPDK with adding these two
> fields. Then test with at least 2 devices with test-bbdev and see if it does not
> crash or fail?
This is not something we would validate really. But I agree the chance of that ABI having an actual impact is slim based on the address alignment.
Still by process, we can use the abi warning above as a litmus the ABI has some minor change.
Also we introduce this change in the new LTS version, so unsure this is controversial or impactful.
Let me know if different opinion.
Thanks
Nic
>
> Thanks,
> Maxime
>
> >
> >>
> >> Maxime
> >>
> >>>
> >>>>
> >>>> Maxime
> >>>>
> >>>>> 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
> >>>>>
> >>>
> >
On 6/15/23 21:30, Chautru, Nicolas wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> On 6/14/23 20:18, Chautru, Nicolas wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com> Hi,
>>>>
>>>> On 6/13/23 19:16, Chautru, Nicolas wrote:
>>>>> Hi Maxime,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>
>>>>>>
>>>>>> On 6/12/23 22:53, Chautru, Nicolas wrote:
>>>>>>> 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.
>>>>>>
>>>>>> If we are going to break the ABI, better to do the right rework
>>>>>> directly. Otherwise we'll end-up breaking it again next year.
>>>>>
>>>>> With the suggested change, this will not break ABI next year. Any
>>>>> future
>>>> functions are added at the end of the structure anyway.
>>>>
>>>> I'm not so sure, it depends if adding a new field at the end cross a
>>>> cacheline boundary or not:
>>>>
>>>> /*
>>>> * Global array of all devices. This is not static because it's used by the
>>>> * inline enqueue and dequeue functions
>>>> */
>>>> struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
>>>>
>>>> If the older inlined functions used by the application retrieve the
>>>> dev pointer from the array directly (they do) and added new fields in
>>>> new version cross a cacheline, then there will be a misalignement
>>>> between the new lib version and the application using the older inlined
>> functions.
>>>>
>>>> ABI-wise, this is not really future proof.
>>>>
>>>>>
>>>>>>
>>>>>> IMHO, moving these ops should be quite trivial and not much work.
>>>>>>
>>>>>> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
>>>>>> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it
>>>>>> may not break the ABI, but that's a bit fragile:
>>>>>> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
>>>>>> should be OK
>>>>>> - struct rte_bbdev is cache-aligned, so it may work if adding these two
>>>>>> ops do not overlap a cacheline which depends on the CPU
>> architecture.
>>>>>
>>>>> If you prefer to add the only 2 new functions at the end of the
>>>>> structure
>>>> that is okay. I believe it would be cleaner to move all these
>>>> enqueue/dequeue funs down together without drawback I can think of.
>>>> Let me know.
>>>>
>>>> Adding the new ones at the end is not future proof, but at least it
>>>> does not break ABI just for cosmetic reasons (that's a big drawback
>> IMHO).
>>>>
>>>> I just checked using pahole:
>>>>
>>>> struct rte_bbdev {
>>>> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
>>>> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
>>>> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
>>>> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
>>>> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8
>>>> */
>>>> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8
>>>> */
>>>> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8
>>>> */
>>>> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8
>>>> */
>>>> /* --- cacheline 1 boundary (64 bytes) --- */
>>>> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
>>>> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
>>>> const struct rte_bbdev_ops * dev_ops; /* 80 8 */
>>>> struct rte_bbdev_data * data; /* 88 8 */
>>>> enum rte_bbdev_state state; /* 96 4 */
>>>>
>>>> /* XXX 4 bytes hole, try to pack */
>>>>
>>>> struct rte_device * device; /* 104 8 */
>>>> struct rte_bbdev_cb_list list_cbs; /* 112 16 */
>>>> /* --- cacheline 2 boundary (128 bytes) --- */
>>>> struct rte_intr_handle * intr_handle; /* 128 8 */
>>>>
>>>> /* size: 192, cachelines: 3, members: 16 */
>>>> /* sum members: 132, holes: 1, sum holes: 4 */
>>>> /* padding: 56 */
>>>> } __attribute__((__aligned__(64)));
>>>>
>>>> We're lucky on x86, we still have 56 bytes, so we can add 7 new ops
>>>> at the end before breaking the ABI if I'm not mistaken.
>>>>
>>>> I checked the other architecture, and it seems we don't support any
>>>> with 32B cacheline size so we're good for a while.
>>>
>>> OK then just adding the new functions at the end, no other cosmetic
>> changes. Will update the patch to match this.
>>> In term of deprecation notice, you are okay with latest draft?
>>>
>>> +* 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.
>>
>> This is not needed in the deprecation notice.
>> If you are willing to announce it, it could be part of the Intel roadmap.
>>
>
> I still see this abi failure as we extend the struct (see below), what is the harm in calling it out in the deprecation notice?
>
> 1 function with some indirect sub-type change:
>
> [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 enqueue_mldts_ops', at offset 1088 (in bits) at rte_bbdev.h:527:1
> 'rte_bbdev_dequeue_mldts_ops_t dequeue_mldts_ops', at offset 1152 (in bits) at rte_bbdev.h:529:1
> no data member changes (12 filtered);
>
> Error: ABI issue reported for abidiff --suppr /home-local/jenkins-local/jenkins-agent/workspace/Generic-DPDK-Compile-ABI@2/dpdk/devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 build_install/usr/local/include reference/usr/local/lib64/librte_bbdev.so.23.0 build_install/usr/local/lib64/librte_bbdev.so.23.2
> ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).
>
>>
>> To be on the safe side, could you try to dynamically link an application with
>> a DPDK version before this change, then rebuild DPDK with adding these two
>> fields. Then test with at least 2 devices with test-bbdev and see if it does not
>> crash or fail?
>
> This is not something we would validate really. But I agree the chance of that ABI having an actual impact is slim based on the address alignment.
Bbdev is not only about Intel AFAICS.
> Still by process, we can use the abi warning above as a litmus the ABI has some minor change.
> Also we introduce this change in the new LTS version, so unsure this is controversial or impactful.
> Let me know if different opinion.
Either we are sure we can waive this warning, e.g. by testing it.
Or we cannot, and in this case we have an ABI break.
If we are going to have an ABI break, let's do the right thing now and
move ops in a dedicated struct as suggested by Stephen, David and
myself.
Maxime
> Thanks
> Nic
>
>
>>
>> Thanks,
>> Maxime
>>
>>>
>>>>
>>>> Maxime
>>>>
>>>>>
>>>>>>
>>>>>> Maxime
>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>
>>>
>
Hi Maxime, Hermant,
Hermant can I have your view on this topic below.
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> On 6/15/23 21:30, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>
> >> On 6/14/23 20:18, Chautru, Nicolas wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com> Hi,
> >>>>
> >>>> On 6/13/23 19:16, Chautru, Nicolas wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>
> >>>>>>
> >>>>>> On 6/12/23 22:53, Chautru, Nicolas wrote:
> >>>>>>> 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.
> >>>>>>
> >>>>>> If we are going to break the ABI, better to do the right rework
> >>>>>> directly. Otherwise we'll end-up breaking it again next year.
> >>>>>
> >>>>> With the suggested change, this will not break ABI next year. Any
> >>>>> future
> >>>> functions are added at the end of the structure anyway.
> >>>>
> >>>> I'm not so sure, it depends if adding a new field at the end cross
> >>>> a cacheline boundary or not:
> >>>>
> >>>> /*
> >>>> * Global array of all devices. This is not static because it's used by the
> >>>> * inline enqueue and dequeue functions
> >>>> */
> >>>> struct rte_bbdev rte_bbdev_devices[RTE_BBDEV_MAX_DEVS];
> >>>>
> >>>> If the older inlined functions used by the application retrieve the
> >>>> dev pointer from the array directly (they do) and added new fields
> >>>> in new version cross a cacheline, then there will be a
> >>>> misalignement between the new lib version and the application using
> >>>> the older inlined
> >> functions.
> >>>>
> >>>> ABI-wise, this is not really future proof.
> >>>>
> >>>>>
> >>>>>>
> >>>>>> IMHO, moving these ops should be quite trivial and not much work.
> >>>>>>
> >>>>>> Otherwise, if we just placed the rte_bbdev_dequeue_mldts_ops and
> >>>>>> rte_bbdev_enqueue_mldts_ops at the bottom of struct rte_bbdev, it
> >>>>>> may not break the ABI, but that's a bit fragile:
> >>>>>> - rte_bbdev_devices[] is not static, but is placed in the BSS section so
> >>>>>> should be OK
> >>>>>> - struct rte_bbdev is cache-aligned, so it may work if adding these two
> >>>>>> ops do not overlap a cacheline which depends on the CPU
> >> architecture.
> >>>>>
> >>>>> If you prefer to add the only 2 new functions at the end of the
> >>>>> structure
> >>>> that is okay. I believe it would be cleaner to move all these
> >>>> enqueue/dequeue funs down together without drawback I can think of.
> >>>> Let me know.
> >>>>
> >>>> Adding the new ones at the end is not future proof, but at least it
> >>>> does not break ABI just for cosmetic reasons (that's a big drawback
> >> IMHO).
> >>>>
> >>>> I just checked using pahole:
> >>>>
> >>>> struct rte_bbdev {
> >>>> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; /* 0 8 */
> >>>> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; /* 8 8 */
> >>>> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; /* 16 8 */
> >>>> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; /* 24 8 */
> >>>> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; /* 32 8
> >>>> */
> >>>> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; /* 40 8
> >>>> */
> >>>> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; /* 48 8
> >>>> */
> >>>> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; /* 56 8
> >>>> */
> >>>> /* --- cacheline 1 boundary (64 bytes) --- */
> >>>> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; /* 64 8 */
> >>>> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; /* 72 8 */
> >>>> const struct rte_bbdev_ops * dev_ops; /* 80 8 */
> >>>> struct rte_bbdev_data * data; /* 88 8 */
> >>>> enum rte_bbdev_state state; /* 96 4 */
> >>>>
> >>>> /* XXX 4 bytes hole, try to pack */
> >>>>
> >>>> struct rte_device * device; /* 104 8 */
> >>>> struct rte_bbdev_cb_list list_cbs; /* 112 16 */
> >>>> /* --- cacheline 2 boundary (128 bytes) --- */
> >>>> struct rte_intr_handle * intr_handle; /* 128 8 */
> >>>>
> >>>> /* size: 192, cachelines: 3, members: 16 */
> >>>> /* sum members: 132, holes: 1, sum holes: 4 */
> >>>> /* padding: 56 */
> >>>> } __attribute__((__aligned__(64)));
> >>>>
> >>>> We're lucky on x86, we still have 56 bytes, so we can add 7 new ops
> >>>> at the end before breaking the ABI if I'm not mistaken.
> >>>>
> >>>> I checked the other architecture, and it seems we don't support any
> >>>> with 32B cacheline size so we're good for a while.
> >>>
> >>> OK then just adding the new functions at the end, no other cosmetic
> >> changes. Will update the patch to match this.
> >>> In term of deprecation notice, you are okay with latest draft?
> >>>
> >>> +* 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.
> >>
> >> This is not needed in the deprecation notice.
> >> If you are willing to announce it, it could be part of the Intel roadmap.
> >>
> >
> > I still see this abi failure as we extend the struct (see below), what is the harm
> in calling it out in the deprecation notice?
> >
> > 1 function with some indirect sub-type change:
> >
> > [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 enqueue_mldts_ops', at offset 1088
> (in bits) at rte_bbdev.h:527:1
> > 'rte_bbdev_dequeue_mldts_ops_t dequeue_mldts_ops', at offset 1152
> (in bits) at rte_bbdev.h:529:1
> > no data member changes (12 filtered);
> >
> > Error: ABI issue reported for abidiff --suppr
> > /home-local/jenkins-local/jenkins-agent/workspace/Generic-DPDK-Compile
> > -ABI@2/dpdk/devtools/libabigail.abignore --no-added-syms
> > --headers-dir1 reference/usr/local/include --headers-dir2
> > build_install/usr/local/include
> > reference/usr/local/lib64/librte_bbdev.so.23.0
> > build_install/usr/local/lib64/librte_bbdev.so.23.2
> > ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a
> potential issue).
> >
> >>
> >> To be on the safe side, could you try to dynamically link an
> >> application with a DPDK version before this change, then rebuild DPDK
> >> with adding these two fields. Then test with at least 2 devices with
> >> test-bbdev and see if it does not crash or fail?
> >
> > This is not something we would validate really. But I agree the chance of that
> ABI having an actual impact is slim based on the address alignment.
>
> Bbdev is not only about Intel AFAICS.
Agreed 100% but I have no way to validate on non-Intel architecture or using non-Intel HW. All I can guarantee is that this would be fine on Intel environment based on the warning below.
Hemant, can you share your view on this? Any concern on the bbdev extension for the new operation (this is similar addition to the FFT one done in 22.11) in your architecture?
Thanks
Nic
>
> > Still by process, we can use the abi warning above as a litmus the ABI has some
> minor change.
> > Also we introduce this change in the new LTS version, so unsure this is
> controversial or impactful.
> > Let me know if different opinion.
>
> Either we are sure we can waive this warning, e.g. by testing it.
> Or we cannot, and in this case we have an ABI break.
> If we are going to have an ABI break, let's do the right thing now and move ops in
> a dedicated struct as suggested by Stephen, David and myself.
>
> Maxime
>
> > Thanks
> > Nic
> >
> >
> >>
> >> Thanks,
> >> Maxime
> >>
> >>>
> >>>>
> >>>> Maxime
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Maxime
> >>>>>>
> >>>>>>> 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
> >>>>>>>
> >>>>>
> >>>
> >
@@ -135,3 +135,9 @@ Deprecation Notices
Its removal has been postponed to let potential users report interest
in maintaining it.
In the absence of such interest, this library will be removed in DPDK 23.11.
+
+* 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 also introduce
+ new symbols for ``rte_bbdev_dequeue_mldts_ops``, ``rte_bbdev_enqueue_mldts_ops``,
+ ``rte_bbdev_mldts_op_alloc_bulk`` and ``rte_bbdev_mldts_op_free_bulk``. This will also extend
+ the API related to the FFT operation in ``rte_bbdev_op_fft``.