[v3,1/5] event/opdl: fix non-constant compile time assertion
Checks
Commit Message
RTE_BUILD_BUG_ON() was being used with a non-constant value.
The inline function rte_is_power_of_2() is not constant since
inline expansion happens later in the compile process.
Replace it with macro which will be constant.
Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
Cc: liang.j.ma@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
drivers/event/opdl/opdl_ring.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On 1/16/24 21:41, Stephen Hemminger wrote:
> RTE_BUILD_BUG_ON() was being used with a non-constant value.
> The inline function rte_is_power_of_2() is not constant since
> inline expansion happens later in the compile process.
> Replace it with macro which will be constant.
>
> Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> Cc: liang.j.ma@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
> drivers/event/opdl/opdl_ring.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
> index 69392b56bbec..24e0bbe3222d 100644
> --- a/drivers/event/opdl/opdl_ring.c
> +++ b/drivers/event/opdl/opdl_ring.c
> @@ -31,6 +31,9 @@
> #define OPDL_OPA_MASK (0xFF)
> #define OPDL_OPA_OFFSET (0x38)
>
> +/* Equivalent to rte_is_power_of_2() but as macro. */
> +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
IMHO adding it in specific driver is a wrong direction. I'm afraid it
will result in duplication of such macros in code base without clear
reason why.
May be it is better to add it with a proper name to EAL?
> +
> /* Types of dependency between stages */
> enum dep_type {
> DEP_NONE = 0, /* no dependency */
> @@ -910,7 +913,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
> RTE_CACHE_LINE_MASK) != 0);
> RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
> RTE_CACHE_LINE_MASK) != 0);
> - RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
> + RTE_BUILD_BUG_ON(!IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
>
> /* Parameter checking */
> if (name == NULL) {
On Wed, Jan 17, 2024 at 10:58:12AM +0300, Andrew Rybchenko wrote:
> On 1/16/24 21:41, Stephen Hemminger wrote:
> > RTE_BUILD_BUG_ON() was being used with a non-constant value.
> > The inline function rte_is_power_of_2() is not constant since
> > inline expansion happens later in the compile process.
> > Replace it with macro which will be constant.
> >
> > Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> > Cc: liang.j.ma@intel.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> > drivers/event/opdl/opdl_ring.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
> > index 69392b56bbec..24e0bbe3222d 100644
> > --- a/drivers/event/opdl/opdl_ring.c
> > +++ b/drivers/event/opdl/opdl_ring.c
> > @@ -31,6 +31,9 @@
> > #define OPDL_OPA_MASK (0xFF)
> > #define OPDL_OPA_OFFSET (0x38)
> > +/* Equivalent to rte_is_power_of_2() but as macro. */
> > +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
>
> IMHO adding it in specific driver is a wrong direction. I'm afraid it
> will result in duplication of such macros in code base without clear
> reason why.
>
> May be it is better to add it with a proper name to EAL?
>
+1
Even if it's a lower-case name, lets make it a macro in EAL so we can use
it everywhere in asserts.
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 17 January 2024 10.27
>
> On Wed, Jan 17, 2024 at 10:58:12AM +0300, Andrew Rybchenko wrote:
> > On 1/16/24 21:41, Stephen Hemminger wrote:
> > > RTE_BUILD_BUG_ON() was being used with a non-constant value.
> > > The inline function rte_is_power_of_2() is not constant since
> > > inline expansion happens later in the compile process.
> > > Replace it with macro which will be constant.
> > >
> > > Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure
> library")
> > > Cc: liang.j.ma@intel.com
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > > drivers/event/opdl/opdl_ring.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/event/opdl/opdl_ring.c
> b/drivers/event/opdl/opdl_ring.c
> > > index 69392b56bbec..24e0bbe3222d 100644
> > > --- a/drivers/event/opdl/opdl_ring.c
> > > +++ b/drivers/event/opdl/opdl_ring.c
> > > @@ -31,6 +31,9 @@
> > > #define OPDL_OPA_MASK (0xFF)
> > > #define OPDL_OPA_OFFSET (0x38)
> > > +/* Equivalent to rte_is_power_of_2() but as macro. */
> > > +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
> >
> > IMHO adding it in specific driver is a wrong direction. I'm afraid it
> > will result in duplication of such macros in code base without clear
> > reason why.
> >
> > May be it is better to add it with a proper name to EAL?
> >
> +1
> Even if it's a lower-case name, lets make it a macro in EAL so we can
> use
> it everywhere in asserts.
Here's a thought... we could introduce a new code convention for this purpose. E.g. either:
1. Macros in lower case are guaranteed to access each parameter exactly once (and might not be usable for static_assert; they somewhat resemble inline functions), and
macros in upper case may access parameters any number of times (and must be usable for static_assert).
Or:
2. Macros that access any parameter zero or multiple times must have their name postfixed _UNSAFE (or some other word).
@@ -31,6 +31,9 @@
#define OPDL_OPA_MASK (0xFF)
#define OPDL_OPA_OFFSET (0x38)
+/* Equivalent to rte_is_power_of_2() but as macro. */
+#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
+
/* Types of dependency between stages */
enum dep_type {
DEP_NONE = 0, /* no dependency */
@@ -910,7 +913,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
RTE_CACHE_LINE_MASK) != 0);
RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
RTE_CACHE_LINE_MASK) != 0);
- RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
+ RTE_BUILD_BUG_ON(!IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
/* Parameter checking */
if (name == NULL) {