[PATCH] eal: Pointer alignment check improvements

[PATCH] eal: Pointer alignment check improvements

[Morten Brørup]

Checking a const pointer for alignment would emit a warning about the
const qualifier being discarded.

No need to calculate the aligned pointer; just check the last bits of the
pointer.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 2e22c1b955..5c20d3a81a 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -403,10 +403,10 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * @return
  *   True(1) where the pointer is correctly aligned, false(0) otherwise
  */
-static inline int
-rte_is_aligned(void *ptr, unsigned align)
+static __rte_always_inline __attribute__ ((const)) int
+rte_is_aligned(const void * const __rte_restrict ptr, unsigned int align)
 {
-	return RTE_PTR_ALIGN(ptr, align) == ptr;
+	return ((uintptr_t)ptr & (align - 1)) == 0;
 }
 
 /*********** Macros for compile type checks ********/
-- 
2.17.1

[PATCH v2] eal: Pointer alignment check improvements

[Morten Brørup]

Checking a const pointer for alignment would emit a warning about the
const qualifier being discarded.

No need to calculate the aligned pointer; just check the last bits of the
pointer.

v2:
- Remove compiler attribute ((const)) from function;
  it was a coding style issue.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 2e22c1b955..ed81e0db0a 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -404,9 +404,9 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  *   True(1) where the pointer is correctly aligned, false(0) otherwise
  */
 static inline int
-rte_is_aligned(void *ptr, unsigned align)
+rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 {
-	return RTE_PTR_ALIGN(ptr, align) == ptr;
+	return ((uintptr_t)ptr & (align - 1)) == 0;
 }
 
 /*********** Macros for compile type checks ********/
-- 
2.17.1

Re: [PATCH v2] eal: Pointer alignment check improvements

[Bruce Richardson]

On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote:
> Checking a const pointer for alignment would emit a warning about the
> const qualifier being discarded.
> 
> No need to calculate the aligned pointer; just check the last bits of the
> pointer.
> 
> v2:
> - Remove compiler attribute ((const)) from function;
>   it was a coding style issue.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/eal/include/rte_common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 2e22c1b955..ed81e0db0a 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -404,9 +404,9 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   *   True(1) where the pointer is correctly aligned, false(0) otherwise
>   */
>  static inline int
> -rte_is_aligned(void *ptr, unsigned align)
> +rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
>  {
> -	return RTE_PTR_ALIGN(ptr, align) == ptr;
> +	return ((uintptr_t)ptr & (align - 1)) == 0;

Are we confident that in future, or using come compiler settings, we won't
get an error due to using "uintptr_t" rather than "const uintptr_t" in the
cast? I would put a const in there myself, just to be safe.

A further point, only-semi-related to this patch, which is fine as-is:
looking at the code for the various macros in rte_common.h:
* The various macros for working on pointers can can probably be converted
  to functions, since they don't need to work with variable-sized types.
* We can then see about properly ensuring those inline functions are
  const-correct.

/Bruce

Re: [PATCH v2] eal: Pointer alignment check improvements

[Bruce Richardson]

On Thu, Sep 22, 2022 at 12:52:42PM +0100, Bruce Richardson wrote:
> On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote:
> > Checking a const pointer for alignment would emit a warning about the
> > const qualifier being discarded.
> > 
> > No need to calculate the aligned pointer; just check the last bits of the
> > pointer.
> > 
> > v2:
> > - Remove compiler attribute ((const)) from function;
> >   it was a coding style issue.
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  lib/eal/include/rte_common.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> > index 2e22c1b955..ed81e0db0a 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -404,9 +404,9 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >   *   True(1) where the pointer is correctly aligned, false(0) otherwise
> >   */
> >  static inline int
> > -rte_is_aligned(void *ptr, unsigned align)
> > +rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
> >  {
> > -	return RTE_PTR_ALIGN(ptr, align) == ptr;
> > +	return ((uintptr_t)ptr & (align - 1)) == 0;
> 
> Are we confident that in future, or using come compiler settings, we won't
> get an error due to using "uintptr_t" rather than "const uintptr_t" in the
> cast? I would put a const in there myself, just to be safe.
> 
> A further point, only-semi-related to this patch, which is fine as-is:
> looking at the code for the various macros in rte_common.h:
> * The various macros for working on pointers can can probably be converted
>   to functions, since they don't need to work with variable-sized types.
> * We can then see about properly ensuring those inline functions are
>   const-correct.
> 
Actually, on further investigation in trying this, it appears that the
macros are used in a number of places with integer data too, despite the
"PTR" in the name, so things are best alone for now, I think.

/Bruce

RE: [PATCH v2] eal: Pointer alignment check improvements

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 22 September 2022 13.59
> 
> On Thu, Sep 22, 2022 at 12:52:42PM +0100, Bruce Richardson wrote:
> > On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote:
> > > Checking a const pointer for alignment would emit a warning about
> the
> > > const qualifier being discarded.
> > >
> > > No need to calculate the aligned pointer; just check the last bits
> of the
> > > pointer.
> > >
> > > v2:
> > > - Remove compiler attribute ((const)) from function;
> > >   it was a coding style issue.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > >  lib/eal/include/rte_common.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/eal/include/rte_common.h
> b/lib/eal/include/rte_common.h
> > > index 2e22c1b955..ed81e0db0a 100644
> > > --- a/lib/eal/include/rte_common.h
> > > +++ b/lib/eal/include/rte_common.h
> > > @@ -404,9 +404,9 @@ static void
> __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > >   *   True(1) where the pointer is correctly aligned, false(0)
> otherwise
> > >   */
> > >  static inline int
> > > -rte_is_aligned(void *ptr, unsigned align)
> > > +rte_is_aligned(const void * const __rte_restrict ptr, const
> unsigned int align)
> > >  {
> > > -	return RTE_PTR_ALIGN(ptr, align) == ptr;
> > > +	return ((uintptr_t)ptr & (align - 1)) == 0;
> >
> > Are we confident that in future, or using come compiler settings, we
> won't
> > get an error due to using "uintptr_t" rather than "const uintptr_t"
> in the
> > cast? I would put a const in there myself, just to be safe.

Good idea.

> >
> > A further point, only-semi-related to this patch, which is fine as-
> is:
> > looking at the code for the various macros in rte_common.h:
> > * The various macros for working on pointers can can probably be
> converted
> >   to functions, since they don't need to work with variable-sized
> types.
> > * We can then see about properly ensuring those inline functions are
> >   const-correct.

The problem with const in a function parameter is the ripple effect: all the underlying functions must also use const.

I generally prefer using const where possible, but the ripple effect often makes it difficult.

> >
> Actually, on further investigation in trying this, it appears that the
> macros are used in a number of places with integer data too, despite
> the
> "PTR" in the name, so things are best alone for now, I think.

Even the macros that also exist without "PTR" in the name? (Example, please.)

Instead of providing multiple macros for essentially doing the same thing to different types, we could use __builtin_choose_expr [1] to support a variety of types in the macros. This built-in can be used as a workaround for not being able to use C++, where the same function name can be used by multiple functions with different parameter types.

[1]: https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

I was also wondering why the rte_is_aligned function doesn't have "ptr" in its name, because it cannot be used for integer types. Changing rte_is_aligned to a macro using __builtin_choose_expr could solve this. But I don't think such a patch will be popular, so I chose to stick with the simple fix.

-Morten

[PATCH v3] eal: Pointer alignment check improvements

[Morten Brørup]

Checking a const pointer for alignment would emit a warning about the
const qualifier being discarded.

No need to calculate the aligned pointer; just check the last bits of the
pointer.

v3:
- Make the uintptr_t const to avoid potential future warnings. (Bruce)
v2:
- Remove compiler attribute ((const)) from function;
  it was a coding style issue.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 2e22c1b955..ed81e0db0a 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -404,9 +404,9 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  *   True(1) where the pointer is correctly aligned, false(0) otherwise
  */
 static inline int
-rte_is_aligned(void *ptr, unsigned align)
+rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 {
-	return RTE_PTR_ALIGN(ptr, align) == ptr;
+	return ((const uintptr_t)ptr & (align - 1)) == 0;
 }
 
 /*********** Macros for compile type checks ********/
-- 
2.17.1

Re: [PATCH v3] eal: Pointer alignment check improvements

[Bruce Richardson]

On Thu, Sep 22, 2022 at 03:27:30PM +0200, Morten Brørup wrote:
> Checking a const pointer for alignment would emit a warning about the
> const qualifier being discarded.
> 
> No need to calculate the aligned pointer; just check the last bits of the
> pointer.
> 
> v3:
> - Make the uintptr_t const to avoid potential future warnings. (Bruce)
> v2:
> - Remove compiler attribute ((const)) from function;
>   it was a coding style issue.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Or perhaps it should be "Const-acked-by: ... " :-)

RE: [PATCH v3] eal: Pointer alignment check improvements

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 22 September 2022 15.39
> 
> On Thu, Sep 22, 2022 at 03:27:30PM +0200, Morten Brørup wrote:
> > Checking a const pointer for alignment would emit a warning about the
> > const qualifier being discarded.
> >
> > No need to calculate the aligned pointer; just check the last bits of
> the
> > pointer.
> >
> > v3:
> > - Make the uintptr_t const to avoid potential future warnings.
> (Bruce)
> > v2:
> > - Remove compiler attribute ((const)) from function;
> >   it was a coding style issue.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> Or perhaps it should be "Const-acked-by: ... " :-)

Unfortunately not as const as expected by both of us...

The v3 build fails at github [1] with:

../lib/eal/include/rte_common.h: In function 'int rte_is_aligned(const void*, unsigned int)':
../lib/eal/include/rte_common.h:409:27: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
  409 |  return ((const uintptr_t)ptr & (align - 1)) == 0;
      |                           ^~~

[1] http://mails.dpdk.org/archives/test-report/2022-September/308604.html

I don't understand what the problem is, so my solution is omitting the const, i.e. rolling back to v2, which doesn't fail building. Unless you can suggest a better solution, Bruce?

I have changed v2 status in Patchwork back to New and v3 to Superseded.

-Morten

Re: [PATCH v3] eal: Pointer alignment check improvements

[Bruce Richardson]

On Thu, Sep 22, 2022 at 10:54:38PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 22 September 2022 15.39
> > 
> > On Thu, Sep 22, 2022 at 03:27:30PM +0200, Morten Brørup wrote:
> > > Checking a const pointer for alignment would emit a warning about the
> > > const qualifier being discarded.
> > >
> > > No need to calculate the aligned pointer; just check the last bits of
> > the
> > > pointer.
> > >
> > > v3:
> > > - Make the uintptr_t const to avoid potential future warnings.
> > (Bruce)
> > > v2:
> > > - Remove compiler attribute ((const)) from function;
> > >   it was a coding style issue.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > Or perhaps it should be "Const-acked-by: ... " :-)
> 
> Unfortunately not as const as expected by both of us...
> 
> The v3 build fails at github [1] with:
> 
> ../lib/eal/include/rte_common.h: In function 'int rte_is_aligned(const void*, unsigned int)':
> ../lib/eal/include/rte_common.h:409:27: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
>   409 |  return ((const uintptr_t)ptr & (align - 1)) == 0;
>       |                           ^~~
> 
> [1] http://mails.dpdk.org/archives/test-report/2022-September/308604.html
> 
> I don't understand what the problem is, so my solution is omitting the const, i.e. rolling back to v2, which doesn't fail building. Unless you can suggest a better solution, Bruce?
> 
> I have changed v2 status in Patchwork back to New and v3 to Superseded.
>
That is fine. I'll ack v2 so.

Re: [PATCH v2] eal: Pointer alignment check improvements

[Bruce Richardson]

On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote:
> Checking a const pointer for alignment would emit a warning about the
> const qualifier being discarded.
> 
> No need to calculate the aligned pointer; just check the last bits of the
> pointer.
> 
> v2:
> - Remove compiler attribute ((const)) from function;
>   it was a coding style issue.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/eal/include/rte_common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 2e22c1b955..ed81e0db0a 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -404,9 +404,9 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>   *   True(1) where the pointer is correctly aligned, false(0) otherwise
>   */
>  static inline int
> -rte_is_aligned(void *ptr, unsigned align)
> +rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
>  {
> -	return RTE_PTR_ALIGN(ptr, align) == ptr;
> +	return ((uintptr_t)ptr & (align - 1)) == 0;
>  }
>  
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v2] eal: Pointer alignment check improvements

[David Marchand]

On Fri, Sep 23, 2022 at 10:25 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote:
> > Checking a const pointer for alignment would emit a warning about the
> > const qualifier being discarded.
> >
> > No need to calculate the aligned pointer; just check the last bits of the
> > pointer.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks.


-- 
David Marchand