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

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

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