eal/x86: fix build on systems with WAITPKG support
Checks
Commit Message
When doing a build for a system with WAITPKG support and a modern
compiler, we get build errors for the "_umonitor" intrinsic, due to the
casting away of the "volatile" on the parameter.
../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
of '_umonitor' discards 'volatile' qualifier from pointer target type
[-Werror=discarded-qualifiers]
113 | _umonitor(pmc->addr);
| ~~~^~~~~~
We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
through "uintptr_t" and thereby remove the volatile without warning.
We also ensure comments are correct for each leg of the
ifdef..else..endif block.
Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
Cc: roretzla@linux.microsoft.com
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Comments
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 25 August 2023 17.29
>
> When doing a build for a system with WAITPKG support and a modern
> compiler, we get build errors for the "_umonitor" intrinsic, due to the
> casting away of the "volatile" on the parameter.
>
> ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> of '_umonitor' discards 'volatile' qualifier from pointer target type
> [-Werror=discarded-qualifiers]
> 113 | _umonitor(pmc->addr);
> | ~~~^~~~~~
>
> We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
> through "uintptr_t" and thereby remove the volatile without warning.
> We also ensure comments are correct for each leg of the
> ifdef..else..endif block.
>
> Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> Cc: roretzla@linux.microsoft.com
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
[...]
> - _umonitor(pmc->addr);
> + /* use RTE_PTR_ADD to cast away "volatile" when using the
> intrinsic */
Yes. Having a comment here is good, so people don't wonder why the magic has been added.
> + _umonitor(RTE_PTR_ADD(pmc->addr, 0));
I think that (void *)(uintptr_t)p is more readable than RTE_PTR_ADD(p, 0), but it's a matter of taste.
Regardless,
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Hello Bruce,
On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> When doing a build for a system with WAITPKG support and a modern
> compiler, we get build errors for the "_umonitor" intrinsic, due to the
> casting away of the "volatile" on the parameter.
>
> ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> of '_umonitor' discards 'volatile' qualifier from pointer target type
> [-Werror=discarded-qualifiers]
> 113 | _umonitor(pmc->addr);
> | ~~~^~~~~~
>
> We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
> through "uintptr_t" and thereby remove the volatile without warning.
> We also ensure comments are correct for each leg of the
> ifdef..else..endif block.
>
> Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> Cc: roretzla@linux.microsoft.com
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
I'm looking for a system with WAITPKG in the RH lab.. so far, no luck.
Do you have a way to force-reproduce this issue? Like some compiler
options forcing support?
On Mon, Aug 28, 2023 at 9:08 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Hello Bruce,
>
> On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > When doing a build for a system with WAITPKG support and a modern
> > compiler, we get build errors for the "_umonitor" intrinsic, due to the
> > casting away of the "volatile" on the parameter.
> >
> > ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> > ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> > of '_umonitor' discards 'volatile' qualifier from pointer target type
> > [-Werror=discarded-qualifiers]
> > 113 | _umonitor(pmc->addr);
> > | ~~~^~~~~~
> >
> > We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
> > through "uintptr_t" and thereby remove the volatile without warning.
> > We also ensure comments are correct for each leg of the
> > ifdef..else..endif block.
> >
> > Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> > Cc: roretzla@linux.microsoft.com
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> I'm looking for a system with WAITPKG in the RH lab.. so far, no luck.
> Do you have a way to force-reproduce this issue? Like some compiler
> options forcing support?
Ah.. reproduced with -Dcpu_instruction_set=sapphirerapids
[52/241] Compiling C object lib/librte_eal.a.p/eal_x86_rte_power_intrinsics.c.o
../lib/eal/x86/rte_power_intrinsics.c: In function ‘rte_power_monitor’:
../lib/eal/x86/rte_power_intrinsics.c:113:22: warning: passing
argument 1 of ‘_umonitor’ discards ‘volatile’ qualifier from pointer
target type [-Wdiscarded-qualifiers]
113 | _umonitor(pmc->addr);
| ~~~^~~~~~
In file included from
/usr/lib/gcc/x86_64-redhat-linux/11/include/x86gprintrin.h:89,
from
/usr/lib/gcc/x86_64-redhat-linux/11/include/immintrin.h:27,
from ../lib/eal/x86/include/rte_rtm.h:8,
from ../lib/eal/x86/rte_power_intrinsics.c:7:
/usr/lib/gcc/x86_64-redhat-linux/11/include/waitpkgintrin.h:39:18:
note: expected ‘void *’ but argument is of type ‘volatile void *’
39 | _umonitor (void *__A)
| ~~~~~~^~~
[241/241] Linking target lib/librte_ethdev.so.24.0
On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> When doing a build for a system with WAITPKG support and a modern
> compiler, we get build errors for the "_umonitor" intrinsic, due to the
> casting away of the "volatile" on the parameter.
>
> ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> of '_umonitor' discards 'volatile' qualifier from pointer target type
> [-Werror=discarded-qualifiers]
> 113 | _umonitor(pmc->addr);
> | ~~~^~~~~~
>
> We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
> through "uintptr_t" and thereby remove the volatile without warning.
As Morten, I prefer an explicit cast (keeping your comments) as it
seems we are exploiting an implementation detail of RTE_PTR_ADD.
> We also ensure comments are correct for each leg of the
> ifdef..else..endif block.
Thanks.. I had fixed other places but I have missed this one.
>
> Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> Cc: roretzla@linux.microsoft.com
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index 4066d1392e..4f0404bfb8 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -103,15 +103,15 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
> rte_spinlock_lock(&s->lock);
> s->monitor_addr = pmc->addr;
>
> - /*
> - * we're using raw byte codes for now as only the newest compiler
> - * versions support this instruction natively.
> - */
> -
> /* set address for UMONITOR */
> #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> - _umonitor(pmc->addr);
> + /* use RTE_PTR_ADD to cast away "volatile" when using the intrinsic */
> + _umonitor(RTE_PTR_ADD(pmc->addr, 0));
> #else
> + /*
> + * we're using raw byte codes for compiler versions which
> + * don't support this instruction natively.
> + */
> asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
> :
> : "D"(pmc->addr));
Tested-by: David Marchand <david.marchand@redhat.com>
An additional question, would Intel CI catch such issue?
Or was it caught only because you are blessed with bleeding edge hw? :-)
On Mon, Aug 28, 2023 at 11:29:05AM +0200, David Marchand wrote:
> On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > When doing a build for a system with WAITPKG support and a modern
> > compiler, we get build errors for the "_umonitor" intrinsic, due to the
> > casting away of the "volatile" on the parameter.
> >
> > ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> > ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> > of '_umonitor' discards 'volatile' qualifier from pointer target type
> > [-Werror=discarded-qualifiers]
> > 113 | _umonitor(pmc->addr);
> > | ~~~^~~~~~
> >
> > We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the pointer
> > through "uintptr_t" and thereby remove the volatile without warning.
>
> As Morten, I prefer an explicit cast (keeping your comments) as it
> seems we are exploiting an implementation detail of RTE_PTR_ADD.
>
Ok, I'll do a respin with explicit cast.
>
> > We also ensure comments are correct for each leg of the
> > ifdef..else..endif block.
>
> Thanks.. I had fixed other places but I have missed this one.
>
>
> >
> > Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> > Cc: roretzla@linux.microsoft.com
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> > index 4066d1392e..4f0404bfb8 100644
> > --- a/lib/eal/x86/rte_power_intrinsics.c
> > +++ b/lib/eal/x86/rte_power_intrinsics.c
> > @@ -103,15 +103,15 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
> > rte_spinlock_lock(&s->lock);
> > s->monitor_addr = pmc->addr;
> >
> > - /*
> > - * we're using raw byte codes for now as only the newest compiler
> > - * versions support this instruction natively.
> > - */
> > -
> > /* set address for UMONITOR */
> > #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> > - _umonitor(pmc->addr);
> > + /* use RTE_PTR_ADD to cast away "volatile" when using the intrinsic */
> > + _umonitor(RTE_PTR_ADD(pmc->addr, 0));
> > #else
> > + /*
> > + * we're using raw byte codes for compiler versions which
> > + * don't support this instruction natively.
> > + */
> > asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
> > :
> > : "D"(pmc->addr));
>
> Tested-by: David Marchand <david.marchand@redhat.com>
>
> An additional question, would Intel CI catch such issue?
> Or was it caught only because you are blessed with bleeding edge hw? :-)
>
Not sure. I would hope so, though.
/Bruce
For humor
#define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))
On Mon, Aug 28, 2023, 12:29 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:
> On Mon, Aug 28, 2023 at 11:29:05AM +0200, David Marchand wrote:
> > On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > When doing a build for a system with WAITPKG support and a modern
> > > compiler, we get build errors for the "_umonitor" intrinsic, due to the
> > > casting away of the "volatile" on the parameter.
> > >
> > > ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> > > ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> > > of '_umonitor' discards 'volatile' qualifier from pointer target type
> > > [-Werror=discarded-qualifiers]
> > > 113 | _umonitor(pmc->addr);
> > > | ~~~^~~~~~
> > >
> > > We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the
> pointer
> > > through "uintptr_t" and thereby remove the volatile without warning.
> >
> > As Morten, I prefer an explicit cast (keeping your comments) as it
> > seems we are exploiting an implementation detail of RTE_PTR_ADD.
> >
>
> Ok, I'll do a respin with explicit cast.
>
> >
> > > We also ensure comments are correct for each leg of the
> > > ifdef..else..endif block.
> >
> > Thanks.. I had fixed other places but I have missed this one.
> >
> >
> > >
> > > Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> > > Cc: roretzla@linux.microsoft.com
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > > lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/lib/eal/x86/rte_power_intrinsics.c
> b/lib/eal/x86/rte_power_intrinsics.c
> > > index 4066d1392e..4f0404bfb8 100644
> > > --- a/lib/eal/x86/rte_power_intrinsics.c
> > > +++ b/lib/eal/x86/rte_power_intrinsics.c
> > > @@ -103,15 +103,15 @@ rte_power_monitor(const struct
> rte_power_monitor_cond *pmc,
> > > rte_spinlock_lock(&s->lock);
> > > s->monitor_addr = pmc->addr;
> > >
> > > - /*
> > > - * we're using raw byte codes for now as only the newest
> compiler
> > > - * versions support this instruction natively.
> > > - */
> > > -
> > > /* set address for UMONITOR */
> > > #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> > > - _umonitor(pmc->addr);
> > > + /* use RTE_PTR_ADD to cast away "volatile" when using the
> intrinsic */
> > > + _umonitor(RTE_PTR_ADD(pmc->addr, 0));
> > > #else
> > > + /*
> > > + * we're using raw byte codes for compiler versions which
> > > + * don't support this instruction natively.
> > > + */
> > > asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
> > > :
> > > : "D"(pmc->addr));
> >
> > Tested-by: David Marchand <david.marchand@redhat.com>
> >
> > An additional question, would Intel CI catch such issue?
> > Or was it caught only because you are blessed with bleeding edge hw? :-)
> >
> Not sure. I would hope so, though.
>
> /Bruce
>
On Mon, Aug 28, 2023 at 12:42:38PM +0200, Stephen Hemminger wrote:
> For humor
> #define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))
Yes, actually thought about that. Was also wondering about making it an
inline function rather than macro, to ensure its only used on pointers, and
to make clear what is being cast away:
static inline void *
rte_cast_no_volatile(volatile void *x)
{
return (void *)(uintptr_t)(x);
}
and similarly we could do a rte_cast_no_const(const void *x).
WDYT?
/Bruce
On 8/28/2023 12:03 PM, Bruce Richardson wrote:
> On Mon, Aug 28, 2023 at 12:42:38PM +0200, Stephen Hemminger wrote:
>> For humor
>> #define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))
>
> Yes, actually thought about that. Was also wondering about making it an
> inline function rather than macro, to ensure its only used on pointers, and
> to make clear what is being cast away:
>
> static inline void *
> rte_cast_no_volatile(volatile void *x)
> {
> return (void *)(uintptr_t)(x);
> }
>
Not as good, without 'castaway' in the API/macro name :)
> and similarly we could do a rte_cast_no_const(const void *x).
>
> WDYT?
>
> /Bruce
On Mon, Aug 28, 2023 at 12:03:40PM +0100, Bruce Richardson wrote:
> On Mon, Aug 28, 2023 at 12:42:38PM +0200, Stephen Hemminger wrote:
> > For humor
> > #define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))
>
> Yes, actually thought about that. Was also wondering about making it an
> inline function rather than macro, to ensure its only used on pointers, and
> to make clear what is being cast away:
>
> static inline void *
> rte_cast_no_volatile(volatile void *x)
> {
> return (void *)(uintptr_t)(x);
> }
>
> and similarly we could do a rte_cast_no_const(const void *x).
>
> WDYT?
since we're introducing humor! now announcing dpdk requires C23 comliant
compiler for typeof_unqual! https://en.cppreference.com/w/c/language/typeof
i like the idea, i like it being inline function you could use the name
'unqual' if you wanted to mimic a name similar to standard C typeof.
it could be 1 function that strips all qualifications or N that strip
specific qualifications, const, volatile and _Atomic not sure which is
best.
ty
>
> /Bruce
@@ -103,15 +103,15 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
rte_spinlock_lock(&s->lock);
s->monitor_addr = pmc->addr;
- /*
- * we're using raw byte codes for now as only the newest compiler
- * versions support this instruction natively.
- */
-
/* set address for UMONITOR */
#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
- _umonitor(pmc->addr);
+ /* use RTE_PTR_ADD to cast away "volatile" when using the intrinsic */
+ _umonitor(RTE_PTR_ADD(pmc->addr, 0));
#else
+ /*
+ * we're using raw byte codes for compiler versions which
+ * don't support this instruction natively.
+ */
asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
:
: "D"(pmc->addr));