[v2] doc: propose correction rte_{bsf, fls} inline functions type use
Checks
Commit Message
The proposal has resulted from request to review [1] the following
functions where there appeared to be inconsistency in return type
or parameter type selections for the following inline functions.
rte_bsf32()
rte_bsf32_safe()
rte_bsf64()
rte_bsf64_safe()
rte_fls_u32()
rte_fls_u64()
rte_log2_u32()
rte_log2_u64()
[1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
doc/guides/rel_notes/deprecation.rst | 7 +++++++
1 file changed, 7 insertions(+)
Comments
15/03/2021 20:34, Tyler Retzlaff:
> The proposal has resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
>
> rte_bsf32()
> rte_bsf32_safe()
> rte_bsf64()
> rte_bsf64_safe()
> rte_fls_u32()
> rte_fls_u64()
> rte_log2_u32()
> rte_log2_u64()
>
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* eal: Fix inline function return and parameter types for rte_{bsf,fls}
> + inline functions to be consistent.
> + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
> + Change ``rte_bsf64`` return type to ``uint32_t`` instead of ``int``.
> + Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
> + Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.
It seems we completely forgot this.
How critical is it?
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, 25 October 2021 21.14
>
> 15/03/2021 20:34, Tyler Retzlaff:
> > The proposal has resulted from request to review [1] the following
> > functions where there appeared to be inconsistency in return type
> > or parameter type selections for the following inline functions.
> >
> > rte_bsf32()
> > rte_bsf32_safe()
> > rte_bsf64()
> > rte_bsf64_safe()
> > rte_fls_u32()
> > rte_fls_u64()
> > rte_log2_u32()
> > rte_log2_u64()
> >
> > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > +* eal: Fix inline function return and parameter types for
> rte_{bsf,fls}
> > + inline functions to be consistent.
> > + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> ``uint32_t``.
> > + Change ``rte_bsf64`` return type to ``uint32_t`` instead of
> ``int``.
> > + Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> ``int``.
> > + Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> ``int``.
>
> It seems we completely forgot this.
> How critical is it?
Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.
Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.
The update's only tangible benefit is API consistency. :-)
-Morten
On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, 25 October 2021 21.14
> >
> > 15/03/2021 20:34, Tyler Retzlaff:
> > > The proposal has resulted from request to review [1] the following
> > > functions where there appeared to be inconsistency in return type
> > > or parameter type selections for the following inline functions.
> > >
> > > rte_bsf32()
> > > rte_bsf32_safe()
> > > rte_bsf64()
> > > rte_bsf64_safe()
> > > rte_fls_u32()
> > > rte_fls_u64()
> > > rte_log2_u32()
> > > rte_log2_u64()
> > >
> > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > +* eal: Fix inline function return and parameter types for
> > rte_{bsf,fls}
> > > + inline functions to be consistent.
> > > + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > ``uint32_t``.
> > > + Change ``rte_bsf64`` return type to ``uint32_t`` instead of
> > ``int``.
> > > + Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > ``int``.
> > > + Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > ``int``.
> >
> > It seems we completely forgot this.
> > How critical is it?
>
our organization as a matter of internal security policy requires these
sorts of things to be fixed. while i didn't see any bugs in the dpdk
code there is an opportunity for users of these functions to
accidentally write code that is prone to integer and buffer overflow
class bugs.
there is no urgency, but why leave things sloppy? though i do wish this
had been responded to in a more timely manner 7 months for something
that should have almost been rubber stamped.
> Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.
>
> Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.
>
> The update's only tangible benefit is API consistency. :-)
>
> -Morten
11/11/2021 05:15, Tyler Retzlaff:
> On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > Sent: Monday, 25 October 2021 21.14
> > >
> > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > The proposal has resulted from request to review [1] the following
> > > > functions where there appeared to be inconsistency in return type
> > > > or parameter type selections for the following inline functions.
> > > >
> > > > rte_bsf32()
> > > > rte_bsf32_safe()
> > > > rte_bsf64()
> > > > rte_bsf64_safe()
> > > > rte_fls_u32()
> > > > rte_fls_u64()
> > > > rte_log2_u32()
> > > > rte_log2_u64()
> > > >
> > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > +* eal: Fix inline function return and parameter types for
> > > rte_{bsf,fls}
> > > > + inline functions to be consistent.
> > > > + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > > ``uint32_t``.
> > > > + Change ``rte_bsf64`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > > + Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > > + Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > > ``int``.
> > >
> > > It seems we completely forgot this.
> > > How critical is it?
> >
>
> our organization as a matter of internal security policy requires these
> sorts of things to be fixed. while i didn't see any bugs in the dpdk
> code there is an opportunity for users of these functions to
> accidentally write code that is prone to integer and buffer overflow
> class bugs.
>
> there is no urgency, but why leave things sloppy? though i do wish this
> had been responded to in a more timely manner 7 months for something
> that should have almost been rubber stamped.
It's difficult to be on all topics.
The best way to avoid such miss is to ping when you see no progress.
So what's next?
They are only inline functions, right? so no ABI breakage.
Is it going to require any change on application-side? I guess no.
Is it acceptable in 21.11-rc3? maybe too late?
Is it acceptable in 22.02?
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 11 November 2021 12.55
>
> 11/11/2021 05:15, Tyler Retzlaff:
> > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> Monjalon
> > > > Sent: Monday, 25 October 2021 21.14
> > > >
> > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > The proposal has resulted from request to review [1] the
> following
> > > > > functions where there appeared to be inconsistency in return
> type
> > > > > or parameter type selections for the following inline
> functions.
> > > > >
> > > > > rte_bsf32()
> > > > > rte_bsf32_safe()
> > > > > rte_bsf64()
> > > > > rte_bsf64_safe()
> > > > > rte_fls_u32()
> > > > > rte_fls_u64()
> > > > > rte_log2_u32()
> > > > > rte_log2_u64()
> > > > >
> > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > >
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > ---
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > +* eal: Fix inline function return and parameter types for
> > > > rte_{bsf,fls}
> > > > > + inline functions to be consistent.
> > > > > + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> to
> > > > ``uint32_t``.
> > > > > + Change ``rte_bsf64`` return type to ``uint32_t`` instead of
> > > > ``int``.
> > > > > + Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > > > + Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > >
> > > > It seems we completely forgot this.
> > > > How critical is it?
> > >
> >
> > our organization as a matter of internal security policy requires
> these
> > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > code there is an opportunity for users of these functions to
> > accidentally write code that is prone to integer and buffer overflow
> > class bugs.
> >
> > there is no urgency, but why leave things sloppy? though i do wish
> this
> > had been responded to in a more timely manner 7 months for something
> > that should have almost been rubber stamped.
>
> It's difficult to be on all topics.
> The best way to avoid such miss is to ping when you see no progress.
>
> So what's next?
> They are only inline functions, right? so no ABI breakage.
> Is it going to require any change on application-side? I guess no.
> Is it acceptable in 21.11-rc3? maybe too late?
> Is it acceptable in 22.02?
If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.
-Morten
On Thu, Nov 11, 2021 at 6:11 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, 11 November 2021 12.55
> >
> > 11/11/2021 05:15, Tyler Retzlaff:
> > > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > Monjalon
> > > > > Sent: Monday, 25 October 2021 21.14
> > > > >
> > > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > > The proposal has resulted from request to review [1] the
> > following
> > > > > > functions where there appeared to be inconsistency in return
> > type
> > > > > > or parameter type selections for the following inline
> > functions.
> > > > > >
> > > > > > rte_bsf32()
> > > > > > rte_bsf32_safe()
> > > > > > rte_bsf64()
> > > > > > rte_bsf64_safe()
> > > > > > rte_fls_u32()
> > > > > > rte_fls_u64()
> > > > > > rte_log2_u32()
> > > > > > rte_log2_u64()
> > > > > >
> > > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > > >
> > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > > > > ---
> > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > +* eal: Fix inline function return and parameter types for
> > > > > rte_{bsf,fls}
> > > > > > + inline functions to be consistent.
> > > > > > + Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> > to
> > > > > ``uint32_t``.
> > > > > > + Change ``rte_bsf64`` return type to ``uint32_t`` instead of
> > > > > ``int``.
> > > > > > + Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > > > + Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > >
> > > > > It seems we completely forgot this.
> > > > > How critical is it?
> > > >
> > >
> > > our organization as a matter of internal security policy requires
> > these
> > > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > > code there is an opportunity for users of these functions to
> > > accidentally write code that is prone to integer and buffer overflow
> > > class bugs.
> > >
> > > there is no urgency, but why leave things sloppy? though i do wish
> > this
> > > had been responded to in a more timely manner 7 months for something
> > > that should have almost been rubber stamped.
> >
> > It's difficult to be on all topics.
> > The best way to avoid such miss is to ping when you see no progress.
> >
> > So what's next?
> > They are only inline functions, right? so no ABI breakage.
> > Is it going to require any change on application-side? I guess no.
> > Is it acceptable in 21.11-rc3? maybe too late?
> > Is it acceptable in 22.02?
>
> If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.
>
> -Morten
>
Tyler, there was no progress on this topic during the past year.
Please could you send the patch to fix API inconsistencies,
so we will merge it in 22.11?
Hi Thomas,
I will see if i can carve out some time budget for it, it's a pretty old
patch so it could take some time to review.
On Wed, Jul 13, 2022 at 12:13:20PM +0200, Thomas Monjalon wrote:
> Tyler, there was no progress on this topic during the past year.
> Please could you send the patch to fix API inconsistencies,
> so we will merge it in 22.11?
>
@@ -17,6 +17,13 @@ Deprecation Notices
* eal: The function ``rte_eal_remote_launch`` will return new error codes
after read or write error on the pipe, instead of calling ``rte_panic``.
+* eal: Fix inline function return and parameter types for rte_{bsf,fls}
+ inline functions to be consistent.
+ Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
+ Change ``rte_bsf64`` return type to ``uint32_t`` instead of ``int``.
+ Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
+ Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.
+
* rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
not allow for writing optimized code for all the CPU architectures supported
in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers