DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] eal/x86: fix build on systems with WAITPKG support
@ 2023-08-25 15:28 Bruce Richardson
  2023-08-25 16:07 ` Morten Brørup
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Bruce Richardson @ 2023-08-25 15:28 UTC (permalink / raw)
  To: dev; +Cc: mb, david.marchand, Bruce Richardson, roretzla

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(-)

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));
-- 
2.39.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-25 15:28 [PATCH] eal/x86: fix build on systems with WAITPKG support Bruce Richardson
@ 2023-08-25 16:07 ` Morten Brørup
  2023-08-28  7:08 ` David Marchand
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Morten Brørup @ 2023-08-25 16:07 UTC (permalink / raw)
  To: Bruce Richardson, dev; +Cc: david.marchand, roretzla

> 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>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-25 15:28 [PATCH] eal/x86: fix build on systems with WAITPKG support Bruce Richardson
  2023-08-25 16:07 ` Morten Brørup
@ 2023-08-28  7:08 ` David Marchand
  2023-08-28  8:05   ` David Marchand
  2023-08-28  9:29 ` David Marchand
  2023-08-28 10:39 ` [PATCH v2] " Bruce Richardson
  3 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2023-08-28  7:08 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, mb, roretzla

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?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28  7:08 ` David Marchand
@ 2023-08-28  8:05   ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2023-08-28  8:05 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, mb, roretzla

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


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-25 15:28 [PATCH] eal/x86: fix build on systems with WAITPKG support Bruce Richardson
  2023-08-25 16:07 ` Morten Brørup
  2023-08-28  7:08 ` David Marchand
@ 2023-08-28  9:29 ` David Marchand
  2023-08-28 10:29   ` Bruce Richardson
  2023-08-28 10:39 ` [PATCH v2] " Bruce Richardson
  3 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2023-08-28  9:29 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, mb, roretzla

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? :-)


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28  9:29 ` David Marchand
@ 2023-08-28 10:29   ` Bruce Richardson
  2023-08-28 10:42     ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2023-08-28 10:29 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, mb, roretzla

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] eal/x86: fix build on systems with WAITPKG support
  2023-08-25 15:28 [PATCH] eal/x86: fix build on systems with WAITPKG support Bruce Richardson
                   ` (2 preceding siblings ...)
  2023-08-28  9:29 ` David Marchand
@ 2023-08-28 10:39 ` Bruce Richardson
  2023-08-28 14:39   ` David Marchand
  3 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2023-08-28 10:39 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, roretzla, Morten Brørup, David Marchand

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 casting 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>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Tested-by: David Marchand <david.marchand@redhat.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..97202e42fc 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);
+	/* cast away "volatile" when using the intrinsic */
+	_umonitor((void *)(uintptr_t)pmc->addr);
 #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));
-- 
2.39.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28 10:29   ` Bruce Richardson
@ 2023-08-28 10:42     ` Stephen Hemminger
  2023-08-28 11:03       ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2023-08-28 10:42 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: David Marchand, dev, Morten Brørup, Tyler Retzlaff

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 4657 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28 10:42     ` Stephen Hemminger
@ 2023-08-28 11:03       ` Bruce Richardson
  2023-08-28 14:31         ` Ferruh Yigit
  2023-08-28 15:56         ` Tyler Retzlaff
  0 siblings, 2 replies; 12+ messages in thread
From: Bruce Richardson @ 2023-08-28 11:03 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Marchand, dev, Morten Brørup, Tyler Retzlaff

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28 11:03       ` Bruce Richardson
@ 2023-08-28 14:31         ` Ferruh Yigit
  2023-08-28 15:56         ` Tyler Retzlaff
  1 sibling, 0 replies; 12+ messages in thread
From: Ferruh Yigit @ 2023-08-28 14:31 UTC (permalink / raw)
  To: Bruce Richardson, Stephen Hemminger
  Cc: David Marchand, dev, Morten Brørup, Tyler Retzlaff

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] eal/x86: fix build on systems with WAITPKG support
  2023-08-28 10:39 ` [PATCH v2] " Bruce Richardson
@ 2023-08-28 14:39   ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2023-08-28 14:39 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, roretzla, Morten Brørup, Stephen Hemminger

On Mon, Aug 28, 2023 at 12:40 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 casting 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>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Tested-by: David Marchand <david.marchand@redhat.com>

Applied to fix build on the main branch, thanks.

We can look at the casting helper as a followup.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] eal/x86: fix build on systems with WAITPKG support
  2023-08-28 11:03       ` Bruce Richardson
  2023-08-28 14:31         ` Ferruh Yigit
@ 2023-08-28 15:56         ` Tyler Retzlaff
  1 sibling, 0 replies; 12+ messages in thread
From: Tyler Retzlaff @ 2023-08-28 15:56 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: Stephen Hemminger, David Marchand, dev, Morten Brørup

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-08-28 15:57 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-25 15:28 [PATCH] eal/x86: fix build on systems with WAITPKG support Bruce Richardson
2023-08-25 16:07 ` Morten Brørup
2023-08-28  7:08 ` David Marchand
2023-08-28  8:05   ` David Marchand
2023-08-28  9:29 ` David Marchand
2023-08-28 10:29   ` Bruce Richardson
2023-08-28 10:42     ` Stephen Hemminger
2023-08-28 11:03       ` Bruce Richardson
2023-08-28 14:31         ` Ferruh Yigit
2023-08-28 15:56         ` Tyler Retzlaff
2023-08-28 10:39 ` [PATCH v2] " Bruce Richardson
2023-08-28 14:39   ` David Marchand

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).