Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coque...@redhat.com>
> > On 6/12/23 22:53, Chautru, Nicolas wrote: > > Hi Maxime, David, > > > >> -----Original Message----- > >> From: Maxime Coquelin <maxime.coque...@redhat.com> > >> > >> On 6/6/23 23:01, Chautru, Nicolas wrote: > >>> Hi David, > >>> > >>>> -----Original Message----- > >>>> From: David Marchand <david.march...@redhat.com>> >> On Mon, Jun > 5, > >>>> 2023 at 10:08 PM Chautru, Nicolas <nicolas.chau...@intel.com> > >>>> wrote: > >>>>> Wrt the MLD functions: these are new into the related serie but > >>>>> still the > >>>> break the ABI since the struct rte_bbdev includes these functions > >>>> hence causing offset changes. > >>>>> > >>>>> Should I then just rephrase as: > >>>>> > >>>>> +* bbdev: Will extend the API to support the new operation type > >>>>> +``RTE_BBDEV_OP_MLDTS`` as per > >>>>> + this `v1 > >>>>> +<https://patches.dpdk.org/project/dpdk/list/?series=28192>`. > >>>>> This > >>>>> + will notably introduce + new symbols for > >>>>> ``rte_bbdev_dequeue_mldts_ops``, +``rte_bbdev_enqueue_mldts_ops`` > >>>>> into the stuct rte_bbdev. > >>>> > >>>> I don't think we need this deprecation notice. > >>>> > >>>> > >>>> Do you need to expose those new mldts ops in rte_bbdev? > >>>> Can't they go to dev_ops? > >>>> If you can't, at least moving those new ops at the end of the > >>>> structure would avoid the breakage on rte_bbdev. > >>> > >>> It would probably be best to move all these ops at the end of the > >>> structure > >> (ie. keep them together). > >>> In that case the deprecation notice would call out that the > >>> rte_bbdev > >> structure content is more generally modified. Probably best for the > >> longer run. > >>> David, Maxime, ok with that option? > >>> > >>> struct __rte_cache_aligned rte_bbdev { > >>> rte_bbdev_enqueue_enc_ops_t enqueue_enc_ops; > >>> rte_bbdev_enqueue_dec_ops_t enqueue_dec_ops; > >>> rte_bbdev_dequeue_enc_ops_t dequeue_enc_ops; > >>> rte_bbdev_dequeue_dec_ops_t dequeue_dec_ops; > >>> rte_bbdev_enqueue_enc_ops_t enqueue_ldpc_enc_ops; > >>> rte_bbdev_enqueue_dec_ops_t enqueue_ldpc_dec_ops; > >>> rte_bbdev_dequeue_enc_ops_t dequeue_ldpc_enc_ops; > >>> rte_bbdev_dequeue_dec_ops_t dequeue_ldpc_dec_ops; > >>> rte_bbdev_enqueue_fft_ops_t enqueue_fft_ops; > >>> rte_bbdev_dequeue_fft_ops_t dequeue_fft_ops; > >>> const struct rte_bbdev_ops *dev_ops; > >>> struct rte_bbdev_data *data; > >>> enum rte_bbdev_state state; > >>> struct rte_device *device; > >>> struct rte_bbdev_cb_list list_cbs; > >>> struct rte_intr_handle *intr_handle; > >>> }; > >> > >> The best thing, as suggested by David, would be to move all the ops > >> out of struct rte_bbdev, as these should not be visible to the application. > > > > That would be quite disruptive across all PMDs and possible perf impact to > validate. I don’t think this is anywhere realistic to consider such a change > in > 23.11. > > I believe moving these function at the end of the structure is a good > compromise to avoid future breakage of rte_bbdev structure with almost > seamless impact (purely a ABI break when moving into 23.11 which is not > avoidable. Retrospectively we should have done that in 22.11 really. > > 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 > >
