From: "Chautru, Nicolas" <nicolas.chautru@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
David Marchand <david.marchand@redhat.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>,
"dev@dpdk.org" <dev@dpdk.org>, "Rix, Tom" <trix@redhat.com>,
"hemant.agrawal@nxp.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
Date: Thu, 15 Jun 2023 19:30:44 +0000 [thread overview]
Message-ID: <BY5PR11MB44513F44E5D4B3F3758FF053F85BA@BY5PR11MB4451.namprd11.prod.outlook.com> (raw)
In-Reply-To: <4968972c-0660-2ba2-c595-37a69de05fd7@redhat.com>
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
> >>>>>
> >>>
> >
next prev parent reply other threads:[~2023-06-15 19:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-26 2:11 [PATCH v1 0/1] doc: accounce change in bbdev extension Nicolas Chautru
2023-05-26 2:11 ` [PATCH v1 1/1] doc: announce change in bbdev api related to operation extension Nicolas Chautru
2023-05-26 3:47 ` Stephen Hemminger
2023-06-05 19:07 ` Maxime Coquelin
2023-06-05 20:08 ` Chautru, Nicolas
2023-06-06 9:20 ` David Marchand
2023-06-06 21:01 ` Chautru, Nicolas
2023-06-08 8:47 ` Maxime Coquelin
2023-06-12 20:53 ` Chautru, Nicolas
2023-06-13 8:14 ` Maxime Coquelin
2023-06-13 17:16 ` Chautru, Nicolas
2023-06-13 20:00 ` Maxime Coquelin
2023-06-13 21:22 ` Stephen Hemminger
2023-06-14 18:18 ` Chautru, Nicolas
2023-06-15 7:52 ` Maxime Coquelin
2023-06-15 19:30 ` Chautru, Nicolas [this message]
2023-06-16 7:36 ` Maxime Coquelin
2023-06-16 15:48 ` Chautru, Nicolas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY5PR11MB44513F44E5D4B3F3758FF053F85BA@BY5PR11MB4451.namprd11.prod.outlook.com \
--to=nicolas.chautru@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=hernan.vargas@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=stephen@networkplumber.org \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).