* [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros
@ 2021-05-09 17:18 pbhagavatula
2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
0 siblings, 2 replies; 19+ messages in thread
From: pbhagavatula @ 2021-05-09 17:18 UTC (permalink / raw)
To: david.marchand; +Cc: dev, Pavan Nikhilesh, stable, Tyler Retzlaff
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside the macro multiple times.
For example:
RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
Here rte_rdtsc() call is expanded multiple times in the macro
causing it to return different values that leads to unintended
side effects.
Also, update common_autotest to detect macro side effects.
Fixes: f56e551485d5 ("eal: add macro to align value to the nearest multiple")
Cc: stable@dpdk.org
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
Retainded original author signoff, split patches as some of the changes
involved in modifying drivers.
app/test/test_common.c | 27 ++++++++++++++++++++++++++-
lib/eal/include/rte_common.h | 28 +++++++++++++++++++---------
2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..0dbb87e741 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -18,6 +18,13 @@
{printf(x "() test failed!\n");\
return -1;}
+static uintptr_t callcount;
+static uintptr_t
+inc_callcount(void)
+{
+ return callcount++;
+}
+
/* this is really a sanity check */
static int
test_macros(int __rte_unused unused_parm)
@@ -29,7 +36,19 @@ test_macros(int __rte_unused unused_parm)
{printf(#x "() test failed!\n");\
return -1;}
+#define TEST_SIDE_EFFECT_2(macro, type1, type2) \
+ do { \
+ callcount = 0; \
+ (void)macro((type1)inc_callcount(), (type2)inc_callcount()); \
+ if (callcount != 2) { \
+ printf(#macro " has side effects: callcount=%u\n", \
+ (unsigned int)callcount); \
+ ret = -1; \
+ } \
+ } while (0)
+
uintptr_t unused = 0;
+ int ret = 0;
RTE_SET_USED(unused);
@@ -47,7 +66,13 @@ test_macros(int __rte_unused unused_parm)
if (strncmp(RTE_STR(test), "test", sizeof("test")))
FAIL_MACRO(RTE_STR);
- return 0;
+ TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
+ TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
+ return ret;
}
static int
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5..a142596587 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -329,27 +329,37 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* value will be of the same type as the first parameter and will be no lower
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_CEIL(v, mul) \
- ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_CEIL(v, mul) \
+ __extension__({ \
+ typeof(v) _vc = (v); \
+ typeof(v) _mc = (mul); \
+ ((_vc + _mc - 1) / _mc) * _mc; \
+ })
/**
* Macro to align a value to the multiple of given value. The resultant
* value will be of the same type as the first parameter and will be no higher
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_FLOOR(v, mul) \
- (((v) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_FLOOR(v, mul) \
+ __extension__({ \
+ typeof(v) _vf = (v); \
+ typeof(v) _mf = (mul); \
+ (_vf / _mf) * _mf; \
+ })
/**
* Macro to align value to the nearest multiple of the given value.
* The resultant value might be greater than or less than the first parameter
* whichever difference is the lowest.
*/
-#define RTE_ALIGN_MUL_NEAR(v, mul) \
- ({ \
- typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \
- typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \
- (ceil - (v)) > ((v) - floor) ? floor : ceil; \
+#define RTE_ALIGN_MUL_NEAR(v, mul) \
+ __extension__({ \
+ typeof(v) _v = (v); \
+ typeof(v) _m = (mul); \
+ typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m); \
+ typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m); \
+ (ceil - _v) > (_v - floor) ? floor : ceil; \
})
/**
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros
2021-05-09 17:18 [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula
@ 2021-05-09 17:18 ` pbhagavatula
2021-05-09 19:39 ` Stephen Hemminger
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
1 sibling, 1 reply; 19+ messages in thread
From: pbhagavatula @ 2021-05-09 17:18 UTC (permalink / raw)
To: david.marchand, Haiyue Wang, Jiawen Wu, Jian Wang,
Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
Cc: dev, Pavan Nikhilesh, stable
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside RTE_*_ALIGN macros.
Update common_autotest to detect macro side effects.
Workaround static arrays relying on RTE_ALIGN macros.
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_common.c | 6 ++++++
drivers/net/e1000/e1000_ethdev.h | 7 ++++---
drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
lib/eal/include/rte_common.h | 17 +++++++++++++----
lib/ethdev/rte_eth_ctrl.h | 5 +++--
6 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 0dbb87e741..9efe3b10f9 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 3b4d9c3ee6..155d825d89 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
};
#define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex filter. */
-#define IGB_FLEX_FILTER_MASK_SIZE \
- (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) / CHAR_BIT)
- /**< mask bytes in flex filter. */
+#define IGB_FLEX_FILTER_MASK_SIZE \
+ (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT - 1), CHAR_BIT) / \
+ CHAR_BIT)
+/**< mask bytes in flex filter. */
/**
* A structure used to define the flex filter entry
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index a0ce18ca24..f2f8b943d2 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -311,8 +311,10 @@ struct ixgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define IXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(IXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define IXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(IXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct ixgbe_ethertype_filter {
diff --git a/drivers/net/txgbe/txgbe_ethdev.h b/drivers/net/txgbe/txgbe_ethdev.h
index 3021933965..05537b34c7 100644
--- a/drivers/net/txgbe/txgbe_ethdev.h
+++ b/drivers/net/txgbe/txgbe_ethdev.h
@@ -219,8 +219,10 @@ struct txgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define TXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(TXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define TXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(TXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct txgbe_ethertype_filter {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index a142596587..6acd067b5c 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -294,8 +294,13 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* point to an address no lower than the first parameter. Second parameter
* must be a power-of-two value.
*/
-#define RTE_PTR_ALIGN_CEIL(ptr, align) \
- RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align) - 1), align)
+#define RTE_PTR_ALIGN_CEIL(ptr, align) \
+ __extension__({ \
+ typeof(ptr) _pc = (ptr); \
+ typeof(align) _ac = (align); \
+ RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac - 1), \
+ _ac); \
+ })
/**
* Macro to align a value to a given power-of-two. The resultant value
@@ -303,8 +308,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* than the first parameter. Second parameter must be a power-of-two
* value.
*/
-#define RTE_ALIGN_CEIL(val, align) \
- RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
+#define RTE_ALIGN_CEIL(val, align) \
+ __extension__({ \
+ typeof(val) _vc = (val); \
+ typeof(val) _ac = (typeof(val))(align); \
+ RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
+ })
/**
* Macro to align a pointer to a given power-of-two. The resultant
diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
index 42652f9cce..863e56170b 100644
--- a/lib/ethdev/rte_eth_ctrl.h
+++ b/lib/ethdev/rte_eth_ctrl.h
@@ -431,8 +431,9 @@ enum rte_fdir_mode {
};
#define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
-#define RTE_FLOW_MASK_ARRAY_SIZE \
- (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
+#define RTE_FLOW_MASK_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1), UINT64_BIT) / \
+ UINT64_BIT)
/**
* A structure used to get the information of flow director filter.
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros
2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
@ 2021-05-09 19:39 ` Stephen Hemminger
2021-05-10 9:50 ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
0 siblings, 1 reply; 19+ messages in thread
From: Stephen Hemminger @ 2021-05-09 19:39 UTC (permalink / raw)
To: pbhagavatula
Cc: david.marchand, Haiyue Wang, Jiawen Wu, Jian Wang,
Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, stable
On Sun, 9 May 2021 22:48:02 +0530
<pbhagavatula@marvell.com> wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Avoid expanding parameters inside RTE_*_ALIGN macros.
> Update common_autotest to detect macro side effects.
> Workaround static arrays relying on RTE_ALIGN macros.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Why not split these up? It looks like the Intel driver and common part
could be separate?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH 2/2] eal: fix side effects in ptr align macros
2021-05-09 19:39 ` Stephen Hemminger
@ 2021-05-10 9:50 ` Pavan Nikhilesh Bhagavatula
2021-05-10 13:34 ` David Marchand
0 siblings, 1 reply; 19+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2021-05-10 9:50 UTC (permalink / raw)
To: Stephen Hemminger
Cc: david.marchand, Haiyue Wang, Jiawen Wu, Jian Wang,
Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, stable
>On Sun, 9 May 2021 22:48:02 +0530
><pbhagavatula@marvell.com> wrote:
>
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Avoid expanding parameters inside RTE_*_ALIGN macros.
>> Update common_autotest to detect macro side effects.
>> Workaround static arrays relying on RTE_ALIGN macros.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>
>Why not split these up? It looks like the Intel driver and common part
>could be separate?
The common changes break intel/mlx5 driver compilation can't separate them.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH 2/2] eal: fix side effects in ptr align macros
2021-05-10 9:50 ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
@ 2021-05-10 13:34 ` David Marchand
0 siblings, 0 replies; 19+ messages in thread
From: David Marchand @ 2021-05-10 13:34 UTC (permalink / raw)
To: Pavan Nikhilesh Bhagavatula
Cc: Stephen Hemminger, Haiyue Wang, Jiawen Wu, Jian Wang,
Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, stable
Hello Pavan,
On Mon, May 10, 2021 at 11:50 AM Pavan Nikhilesh Bhagavatula
<pbhagavatula@marvell.com> wrote:
>
> >On Sun, 9 May 2021 22:48:02 +0530
> ><pbhagavatula@marvell.com> wrote:
> >
> >> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>
> >> Avoid expanding parameters inside RTE_*_ALIGN macros.
> >> Update common_autotest to detect macro side effects.
> >> Workaround static arrays relying on RTE_ALIGN macros.
> >>
> >> Fixes: af75078fece3 ("first public release")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >
> >Why not split these up? It looks like the Intel driver and common part
> >could be separate?
>
> The common changes break intel/mlx5 driver compilation can't separate them.
Thanks for working on this.
The CI still reports a build error in mlx5.
--
David Marchand
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros
2021-05-09 17:18 [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
@ 2021-05-10 14:02 ` pbhagavatula
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
1 sibling, 2 replies; 19+ messages in thread
From: pbhagavatula @ 2021-05-10 14:02 UTC (permalink / raw)
To: david.marchand; +Cc: dev, Pavan Nikhilesh, stable, Tyler Retzlaff
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside the macro multiple times.
For example:
RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
Here rte_rdtsc() call is expanded multiple times in the macro
causing it to return different values that leads to unintended
side effects.
Also, update common_autotest to detect macro side effects.
Fixes: f56e551485d5 ("eal: add macro to align value to the nearest multiple")
Cc: stable@dpdk.org
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
v2 Changes:
- Fix mlx5 compilation.
app/test/test_common.c | 27 ++++++++++++++++++++++++++-
lib/eal/include/rte_common.h | 28 +++++++++++++++++++---------
2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..0dbb87e741 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -18,6 +18,13 @@
{printf(x "() test failed!\n");\
return -1;}
+static uintptr_t callcount;
+static uintptr_t
+inc_callcount(void)
+{
+ return callcount++;
+}
+
/* this is really a sanity check */
static int
test_macros(int __rte_unused unused_parm)
@@ -29,7 +36,19 @@ test_macros(int __rte_unused unused_parm)
{printf(#x "() test failed!\n");\
return -1;}
+#define TEST_SIDE_EFFECT_2(macro, type1, type2) \
+ do { \
+ callcount = 0; \
+ (void)macro((type1)inc_callcount(), (type2)inc_callcount()); \
+ if (callcount != 2) { \
+ printf(#macro " has side effects: callcount=%u\n", \
+ (unsigned int)callcount); \
+ ret = -1; \
+ } \
+ } while (0)
+
uintptr_t unused = 0;
+ int ret = 0;
RTE_SET_USED(unused);
@@ -47,7 +66,13 @@ test_macros(int __rte_unused unused_parm)
if (strncmp(RTE_STR(test), "test", sizeof("test")))
FAIL_MACRO(RTE_STR);
- return 0;
+ TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
+ TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
+ return ret;
}
static int
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5..a142596587 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -329,27 +329,37 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* value will be of the same type as the first parameter and will be no lower
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_CEIL(v, mul) \
- ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_CEIL(v, mul) \
+ __extension__({ \
+ typeof(v) _vc = (v); \
+ typeof(v) _mc = (mul); \
+ ((_vc + _mc - 1) / _mc) * _mc; \
+ })
/**
* Macro to align a value to the multiple of given value. The resultant
* value will be of the same type as the first parameter and will be no higher
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_FLOOR(v, mul) \
- (((v) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_FLOOR(v, mul) \
+ __extension__({ \
+ typeof(v) _vf = (v); \
+ typeof(v) _mf = (mul); \
+ (_vf / _mf) * _mf; \
+ })
/**
* Macro to align value to the nearest multiple of the given value.
* The resultant value might be greater than or less than the first parameter
* whichever difference is the lowest.
*/
-#define RTE_ALIGN_MUL_NEAR(v, mul) \
- ({ \
- typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \
- typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \
- (ceil - (v)) > ((v) - floor) ? floor : ceil; \
+#define RTE_ALIGN_MUL_NEAR(v, mul) \
+ __extension__({ \
+ typeof(v) _v = (v); \
+ typeof(v) _m = (mul); \
+ typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m); \
+ typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m); \
+ (ceil - _v) > (_v - floor) ? floor : ceil; \
})
/**
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
@ 2021-05-10 14:02 ` pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
1 sibling, 0 replies; 19+ messages in thread
From: pbhagavatula @ 2021-05-10 14:02 UTC (permalink / raw)
To: david.marchand, Haiyue Wang, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Jiawen Wu, Jian Wang, Thomas Monjalon,
Ferruh Yigit, Andrew Rybchenko
Cc: dev, Pavan Nikhilesh, stable
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside RTE_*_ALIGN macros.
Update common_autotest to detect macro side effects.
Workaround static arrays relying on RTE_ALIGN macros.
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_common.c | 6 ++++++
drivers/net/e1000/e1000_ethdev.h | 7 ++++---
drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
drivers/net/mlx5/mlx5_rxtx_vec.h | 7 ++++---
drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
lib/eal/include/rte_common.h | 17 +++++++++++++----
lib/ethdev/rte_eth_ctrl.h | 5 +++--
7 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 0dbb87e741..9efe3b10f9 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 3b4d9c3ee6..155d825d89 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
};
#define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex filter. */
-#define IGB_FLEX_FILTER_MASK_SIZE \
- (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) / CHAR_BIT)
- /**< mask bytes in flex filter. */
+#define IGB_FLEX_FILTER_MASK_SIZE \
+ (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT - 1), CHAR_BIT) / \
+ CHAR_BIT)
+/**< mask bytes in flex filter. */
/**
* A structure used to define the flex filter entry
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index a0ce18ca24..f2f8b943d2 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -311,8 +311,10 @@ struct ixgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define IXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(IXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define IXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(IXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct ixgbe_ethertype_filter {
diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index 93b4f517bb..5d105db474 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -42,8 +42,8 @@ S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, hash) ==
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, ol_flags) ==
offsetof(struct rte_mbuf, rearm_data) + 8);
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, rearm_data) ==
- RTE_ALIGN(offsetof(struct rte_mbuf, rearm_data), 16));
-
+ RTE_ALIGN_FLOOR(offsetof(struct rte_mbuf, rearm_data) + 15,
+ 16));
/* rxq_burst_v() */
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, pkt_len) ==
offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -63,7 +63,8 @@ S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, vlan_info) ==
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, lro_num_seg) + 12 ==
offsetof(struct mlx5_cqe, byte_cnt));
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, sop_drop_qpn) ==
- RTE_ALIGN(offsetof(struct mlx5_cqe, sop_drop_qpn), 8));
+ RTE_ALIGN_FLOOR(offsetof(struct mlx5_cqe, sop_drop_qpn) + 7,
+ 8));
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, op_own) ==
offsetof(struct mlx5_cqe, sop_drop_qpn) + 7);
diff --git a/drivers/net/txgbe/txgbe_ethdev.h b/drivers/net/txgbe/txgbe_ethdev.h
index 3021933965..05537b34c7 100644
--- a/drivers/net/txgbe/txgbe_ethdev.h
+++ b/drivers/net/txgbe/txgbe_ethdev.h
@@ -219,8 +219,10 @@ struct txgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define TXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(TXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define TXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(TXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct txgbe_ethertype_filter {
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index a142596587..6acd067b5c 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -294,8 +294,13 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* point to an address no lower than the first parameter. Second parameter
* must be a power-of-two value.
*/
-#define RTE_PTR_ALIGN_CEIL(ptr, align) \
- RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align) - 1), align)
+#define RTE_PTR_ALIGN_CEIL(ptr, align) \
+ __extension__({ \
+ typeof(ptr) _pc = (ptr); \
+ typeof(align) _ac = (align); \
+ RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac - 1), \
+ _ac); \
+ })
/**
* Macro to align a value to a given power-of-two. The resultant value
@@ -303,8 +308,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* than the first parameter. Second parameter must be a power-of-two
* value.
*/
-#define RTE_ALIGN_CEIL(val, align) \
- RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
+#define RTE_ALIGN_CEIL(val, align) \
+ __extension__({ \
+ typeof(val) _vc = (val); \
+ typeof(val) _ac = (typeof(val))(align); \
+ RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
+ })
/**
* Macro to align a pointer to a given power-of-two. The resultant
diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
index 42652f9cce..863e56170b 100644
--- a/lib/ethdev/rte_eth_ctrl.h
+++ b/lib/ethdev/rte_eth_ctrl.h
@@ -431,8 +431,9 @@ enum rte_fdir_mode {
};
#define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
-#define RTE_FLOW_MASK_ARRAY_SIZE \
- (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
+#define RTE_FLOW_MASK_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1), UINT64_BIT) / \
+ UINT64_BIT)
/**
* A structure used to get the information of flow director filter.
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula
@ 2021-05-10 19:40 ` pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
` (2 more replies)
1 sibling, 3 replies; 19+ messages in thread
From: pbhagavatula @ 2021-05-10 19:40 UTC (permalink / raw)
To: david.marchand; +Cc: dev, Pavan Nikhilesh, stable, Tyler Retzlaff
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside the macro multiple times.
For example:
RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
Here rte_rdtsc() call is expanded multiple times in the macro
causing it to return different values that leads to unintended
side effects.
Also, update common_autotest to detect macro side effects.
Fixes: f56e551485d5 ("eal: add macro to align value to the nearest multiple")
Cc: stable@dpdk.org
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
v3 Changes:
- Fix examples/bbdev_app compilation.
v2 Changes:
- Fix mlx5 compilation.
app/test/test_common.c | 27 ++++++++++++++++++++++++++-
lib/eal/include/rte_common.h | 28 +++++++++++++++++++---------
2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..0dbb87e741 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -18,6 +18,13 @@
{printf(x "() test failed!\n");\
return -1;}
+static uintptr_t callcount;
+static uintptr_t
+inc_callcount(void)
+{
+ return callcount++;
+}
+
/* this is really a sanity check */
static int
test_macros(int __rte_unused unused_parm)
@@ -29,7 +36,19 @@ test_macros(int __rte_unused unused_parm)
{printf(#x "() test failed!\n");\
return -1;}
+#define TEST_SIDE_EFFECT_2(macro, type1, type2) \
+ do { \
+ callcount = 0; \
+ (void)macro((type1)inc_callcount(), (type2)inc_callcount()); \
+ if (callcount != 2) { \
+ printf(#macro " has side effects: callcount=%u\n", \
+ (unsigned int)callcount); \
+ ret = -1; \
+ } \
+ } while (0)
+
uintptr_t unused = 0;
+ int ret = 0;
RTE_SET_USED(unused);
@@ -47,7 +66,13 @@ test_macros(int __rte_unused unused_parm)
if (strncmp(RTE_STR(test), "test", sizeof("test")))
FAIL_MACRO(RTE_STR);
- return 0;
+ TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
+ TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
+ return ret;
}
static int
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5..a142596587 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -329,27 +329,37 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* value will be of the same type as the first parameter and will be no lower
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_CEIL(v, mul) \
- ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_CEIL(v, mul) \
+ __extension__({ \
+ typeof(v) _vc = (v); \
+ typeof(v) _mc = (mul); \
+ ((_vc + _mc - 1) / _mc) * _mc; \
+ })
/**
* Macro to align a value to the multiple of given value. The resultant
* value will be of the same type as the first parameter and will be no higher
* than the first parameter.
*/
-#define RTE_ALIGN_MUL_FLOOR(v, mul) \
- (((v) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_FLOOR(v, mul) \
+ __extension__({ \
+ typeof(v) _vf = (v); \
+ typeof(v) _mf = (mul); \
+ (_vf / _mf) * _mf; \
+ })
/**
* Macro to align value to the nearest multiple of the given value.
* The resultant value might be greater than or less than the first parameter
* whichever difference is the lowest.
*/
-#define RTE_ALIGN_MUL_NEAR(v, mul) \
- ({ \
- typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \
- typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \
- (ceil - (v)) > ((v) - floor) ? floor : ceil; \
+#define RTE_ALIGN_MUL_NEAR(v, mul) \
+ __extension__({ \
+ typeof(v) _v = (v); \
+ typeof(v) _m = (mul); \
+ typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m); \
+ typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m); \
+ (ceil - _v) > (_v - floor) ? floor : ceil; \
})
/**
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
@ 2021-05-10 19:40 ` pbhagavatula
2021-05-11 2:12 ` Wang, Haiyue
2023-07-03 18:55 ` [PATCH] eal: avoid issues in macro expansion of alignment Stephen Hemminger
2023-07-04 16:01 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger
2 siblings, 1 reply; 19+ messages in thread
From: pbhagavatula @ 2021-05-10 19:40 UTC (permalink / raw)
To: david.marchand, Haiyue Wang, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Jiawen Wu, Jian Wang, Nicolas Chautru,
Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
Cc: dev, Pavan Nikhilesh, stable
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Avoid expanding parameters inside RTE_*_ALIGN macros.
Update common_autotest to detect macro side effects.
Workaround static arrays relying on RTE_ALIGN macros.
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_common.c | 6 ++++++
drivers/net/e1000/e1000_ethdev.h | 7 ++++---
drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
drivers/net/mlx5/mlx5_rxtx_vec.h | 7 ++++---
drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
examples/bbdev_app/main.c | 2 +-
lib/eal/include/rte_common.h | 17 +++++++++++++----
lib/ethdev/rte_eth_ctrl.h | 5 +++--
8 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/app/test/test_common.c b/app/test/test_common.c
index 0dbb87e741..9efe3b10f9 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned int);
+ TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 3b4d9c3ee6..155d825d89 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
};
#define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex filter. */
-#define IGB_FLEX_FILTER_MASK_SIZE \
- (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) / CHAR_BIT)
- /**< mask bytes in flex filter. */
+#define IGB_FLEX_FILTER_MASK_SIZE \
+ (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT - 1), CHAR_BIT) / \
+ CHAR_BIT)
+/**< mask bytes in flex filter. */
/**
* A structure used to define the flex filter entry
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index a0ce18ca24..f2f8b943d2 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -311,8 +311,10 @@ struct ixgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define IXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(IXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define IXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(IXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct ixgbe_ethertype_filter {
diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index 93b4f517bb..5d105db474 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -42,8 +42,8 @@ S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, hash) ==
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, ol_flags) ==
offsetof(struct rte_mbuf, rearm_data) + 8);
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, rearm_data) ==
- RTE_ALIGN(offsetof(struct rte_mbuf, rearm_data), 16));
-
+ RTE_ALIGN_FLOOR(offsetof(struct rte_mbuf, rearm_data) + 15,
+ 16));
/* rxq_burst_v() */
S_ASSERT_RTE_MBUF(offsetof(struct rte_mbuf, pkt_len) ==
offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -63,7 +63,8 @@ S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, vlan_info) ==
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, lro_num_seg) + 12 ==
offsetof(struct mlx5_cqe, byte_cnt));
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, sop_drop_qpn) ==
- RTE_ALIGN(offsetof(struct mlx5_cqe, sop_drop_qpn), 8));
+ RTE_ALIGN_FLOOR(offsetof(struct mlx5_cqe, sop_drop_qpn) + 7,
+ 8));
S_ASSERT_MLX5_CQE(offsetof(struct mlx5_cqe, op_own) ==
offsetof(struct mlx5_cqe, sop_drop_qpn) + 7);
diff --git a/drivers/net/txgbe/txgbe_ethdev.h b/drivers/net/txgbe/txgbe_ethdev.h
index 3021933965..05537b34c7 100644
--- a/drivers/net/txgbe/txgbe_ethdev.h
+++ b/drivers/net/txgbe/txgbe_ethdev.h
@@ -219,8 +219,10 @@ struct txgbe_5tuple_filter {
uint16_t queue; /* rx queue assigned to */
};
-#define TXGBE_5TUPLE_ARRAY_SIZE \
- (RTE_ALIGN(TXGBE_MAX_FTQF_FILTERS, (sizeof(uint32_t) * NBBY)) / \
+#define TXGBE_5TUPLE_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(TXGBE_MAX_FTQF_FILTERS + (sizeof(uint32_t) * NBBY) - \
+ 1, \
+ (sizeof(uint32_t) * NBBY)) / \
(sizeof(uint32_t) * NBBY))
struct txgbe_ethertype_filter {
diff --git a/examples/bbdev_app/main.c b/examples/bbdev_app/main.c
index 5251db0b16..3f8d2bb671 100644
--- a/examples/bbdev_app/main.c
+++ b/examples/bbdev_app/main.c
@@ -45,7 +45,7 @@
/* Hardcoded K value */
#define K 40
-#define NCB (3 * RTE_ALIGN_CEIL(K + 4, 32))
+#define NCB (3 * RTE_ALIGN_FLOOR((K + 4) + 31, 32))
#define CRC_24B_LEN 3
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index a142596587..6acd067b5c 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -294,8 +294,13 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* point to an address no lower than the first parameter. Second parameter
* must be a power-of-two value.
*/
-#define RTE_PTR_ALIGN_CEIL(ptr, align) \
- RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align) - 1), align)
+#define RTE_PTR_ALIGN_CEIL(ptr, align) \
+ __extension__({ \
+ typeof(ptr) _pc = (ptr); \
+ typeof(align) _ac = (align); \
+ RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac - 1), \
+ _ac); \
+ })
/**
* Macro to align a value to a given power-of-two. The resultant value
@@ -303,8 +308,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
* than the first parameter. Second parameter must be a power-of-two
* value.
*/
-#define RTE_ALIGN_CEIL(val, align) \
- RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
+#define RTE_ALIGN_CEIL(val, align) \
+ __extension__({ \
+ typeof(val) _vc = (val); \
+ typeof(val) _ac = (typeof(val))(align); \
+ RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
+ })
/**
* Macro to align a pointer to a given power-of-two. The resultant
diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
index 42652f9cce..863e56170b 100644
--- a/lib/ethdev/rte_eth_ctrl.h
+++ b/lib/ethdev/rte_eth_ctrl.h
@@ -431,8 +431,9 @@ enum rte_fdir_mode {
};
#define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
-#define RTE_FLOW_MASK_ARRAY_SIZE \
- (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
+#define RTE_FLOW_MASK_ARRAY_SIZE \
+ (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1), UINT64_BIT) / \
+ UINT64_BIT)
/**
* A structure used to get the information of flow director filter.
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
@ 2021-05-11 2:12 ` Wang, Haiyue
2021-05-11 8:44 ` Pavan Nikhilesh Bhagavatula
0 siblings, 1 reply; 19+ messages in thread
From: Wang, Haiyue @ 2021-05-11 2:12 UTC (permalink / raw)
To: pbhagavatula, david.marchand, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Jiawen Wu, Jian Wang, Chautru, Nicolas,
Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko
Cc: dev, stable
> -----Original Message-----
> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> Sent: Tuesday, May 11, 2021 03:40
> To: david.marchand@redhat.com; Wang, Haiyue <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
> Shahaf Shuler <shahafs@nvidia.com>; Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Jiawen Wu
> <jiawenwu@trustnetic.com>; Jian Wang <jianwang@trustnetic.com>; Chautru, Nicolas
> <nicolas.chautru@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@marvell.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Avoid expanding parameters inside RTE_*_ALIGN macros.
> Update common_autotest to detect macro side effects.
> Workaround static arrays relying on RTE_ALIGN macros.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> app/test/test_common.c | 6 ++++++
> drivers/net/e1000/e1000_ethdev.h | 7 ++++---
> drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
> drivers/net/mlx5/mlx5_rxtx_vec.h | 7 ++++---
> drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
> examples/bbdev_app/main.c | 2 +-
> lib/eal/include/rte_common.h | 17 +++++++++++++----
> lib/ethdev/rte_eth_ctrl.h | 5 +++--
> 8 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/app/test/test_common.c b/app/test/test_common.c
> index 0dbb87e741..9efe3b10f9 100644
> --- a/app/test/test_common.c
> +++ b/app/test/test_common.c
> @@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
> TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
> TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
> TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
> + TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
> + TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned int);
> + TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
> diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
> index 3b4d9c3ee6..155d825d89 100644
> --- a/drivers/net/e1000/e1000_ethdev.h
> +++ b/drivers/net/e1000/e1000_ethdev.h
> @@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
> };
>
> #define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex filter. */
> -#define IGB_FLEX_FILTER_MASK_SIZE \
> - (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) / CHAR_BIT)
> - /**< mask bytes in flex filter. */
> +#define IGB_FLEX_FILTER_MASK_SIZE \
> + (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT - 1), CHAR_BIT) / \
> + CHAR_BIT)
> +/**< mask bytes in flex filter. */
>
Since:
RTE_ALIGN --> RTE_ALIGN_CEIL(val, align) --> RTE_ALIGN_FLOOR(...)
Why only change 'RTE_ALIGN_CEIL', but then call 'RTE_ALIGN_FLOOR' directly ?
Sorry, can't get the point. : - (
Why not change 'RTE_ALIGN' if it has issue ?
>
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index a142596587..6acd067b5c 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -294,8 +294,13 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> * point to an address no lower than the first parameter. Second parameter
> * must be a power-of-two value.
> */
> -#define RTE_PTR_ALIGN_CEIL(ptr, align) \
> - RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align) - 1), align)
> +#define RTE_PTR_ALIGN_CEIL(ptr, align) \
> + __extension__({ \
> + typeof(ptr) _pc = (ptr); \
> + typeof(align) _ac = (align); \
> + RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac - 1), \
> + _ac); \
> + })
>
> /**
> * Macro to align a value to a given power-of-two. The resultant value
> @@ -303,8 +308,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> * than the first parameter. Second parameter must be a power-of-two
> * value.
> */
> -#define RTE_ALIGN_CEIL(val, align) \
> - RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
> +#define RTE_ALIGN_CEIL(val, align) \
> + __extension__({ \
> + typeof(val) _vc = (val); \
> + typeof(val) _ac = (typeof(val))(align); \
> + RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
> + })
>
> /**
> * Macro to align a pointer to a given power-of-two. The resultant
> diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
> index 42652f9cce..863e56170b 100644
> --- a/lib/ethdev/rte_eth_ctrl.h
> +++ b/lib/ethdev/rte_eth_ctrl.h
> @@ -431,8 +431,9 @@ enum rte_fdir_mode {
> };
>
> #define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
> -#define RTE_FLOW_MASK_ARRAY_SIZE \
> - (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
> +#define RTE_FLOW_MASK_ARRAY_SIZE \
> + (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1), UINT64_BIT) / \
> + UINT64_BIT)
>
> /**
> * A structure used to get the information of flow director filter.
> --
> 2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
2021-05-11 2:12 ` Wang, Haiyue
@ 2021-05-11 8:44 ` Pavan Nikhilesh Bhagavatula
2021-05-12 1:13 ` Wang, Haiyue
0 siblings, 1 reply; 19+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2021-05-11 8:44 UTC (permalink / raw)
To: Wang, Haiyue, david.marchand, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Jiawen Wu, Jian Wang, Chautru, Nicolas,
Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko
Cc: dev, stable
>> -----Original Message-----
>> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
>> Sent: Tuesday, May 11, 2021 03:40
>> To: david.marchand@redhat.com; Wang, Haiyue
><haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
>> Shahaf Shuler <shahafs@nvidia.com>; Viacheslav Ovsiienko
><viacheslavo@nvidia.com>; Jiawen Wu
>> <jiawenwu@trustnetic.com>; Jian Wang <jianwang@trustnetic.com>;
>Chautru, Nicolas
>> <nicolas.chautru@intel.com>; Thomas Monjalon
><thomas@monjalon.net>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>
>> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@marvell.com>;
>stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align
>macros
>>
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Avoid expanding parameters inside RTE_*_ALIGN macros.
>> Update common_autotest to detect macro side effects.
>> Workaround static arrays relying on RTE_ALIGN macros.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>> app/test/test_common.c | 6 ++++++
>> drivers/net/e1000/e1000_ethdev.h | 7 ++++---
>> drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
>> drivers/net/mlx5/mlx5_rxtx_vec.h | 7 ++++---
>> drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
>> examples/bbdev_app/main.c | 2 +-
>> lib/eal/include/rte_common.h | 17 +++++++++++++----
>> lib/ethdev/rte_eth_ctrl.h | 5 +++--
>> 8 files changed, 39 insertions(+), 17 deletions(-)
>>
>> diff --git a/app/test/test_common.c b/app/test/test_common.c
>> index 0dbb87e741..9efe3b10f9 100644
>> --- a/app/test/test_common.c
>> +++ b/app/test/test_common.c
>> @@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
>> TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
>> TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
>> TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
>> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
>> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
>> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
>> + TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
>> + TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned
>int);
>> + TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int,
>unsigned int);
>> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int,
>unsigned int);
>> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int,
>unsigned int);
>> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int,
>unsigned int);
>> diff --git a/drivers/net/e1000/e1000_ethdev.h
>b/drivers/net/e1000/e1000_ethdev.h
>> index 3b4d9c3ee6..155d825d89 100644
>> --- a/drivers/net/e1000/e1000_ethdev.h
>> +++ b/drivers/net/e1000/e1000_ethdev.h
>> @@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
>> };
>>
>> #define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex
>filter. */
>> -#define IGB_FLEX_FILTER_MASK_SIZE \
>> - (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) /
>CHAR_BIT)
>> - /**< mask bytes in flex filter. */
>> +#define IGB_FLEX_FILTER_MASK_SIZE \
>> + (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT -
>1), CHAR_BIT) / \
>> + CHAR_BIT)
>> +/**< mask bytes in flex filter. */
>>
>
>Since:
>RTE_ALIGN --> RTE_ALIGN_CEIL(val, align) --> RTE_ALIGN_FLOOR(...)
>
>Why only change 'RTE_ALIGN_CEIL', but then call 'RTE_ALIGN_FLOOR'
>directly ?
>Sorry, can't get the point. : - (
>
RTE_ALIGN_CEIL is prone to macro side effects since it uses align twice in the macro.
To fix it RTE_ALIGN_CEIL first expands the val and align values to temporary values
and then calls ALIGN_FLOOR.
Due to this we can no longer use ALIGN/ALIGN_CEIL in static array declarations
Example:
In file included from ../lib/eal/x86/include/rte_atomic.h:13,
from ../examples/bbdev_app/main.c:21:
../lib/eal/include/rte_common.h:312:15: error: braced-group within expression allowed only inside a function
__extension__({ \
^
../examples/bbdev_app/main.c:48:18: note: in expansion of macro 'RTE_ALIGN_CEIL'
#define NCB (3 * RTE_ALIGN_CEIL(K + 4, 32))
^~~~~~~~~~~~~~
../examples/bbdev_app/main.c:145:23: note: in expansion of macro 'NCB'
uint8_t llr_temp_buf[NCB];
To workaround this I had to call RTE_ALIGN_FLOOR for known constants directly.
>Why not change 'RTE_ALIGN' if it has issue ?
>
>>
>> diff --git a/lib/eal/include/rte_common.h
>b/lib/eal/include/rte_common.h
>> index a142596587..6acd067b5c 100644
>> --- a/lib/eal/include/rte_common.h
>> +++ b/lib/eal/include/rte_common.h
>> @@ -294,8 +294,13 @@ static void
>__attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>> * point to an address no lower than the first parameter. Second
>parameter
>> * must be a power-of-two value.
>> */
>> -#define RTE_PTR_ALIGN_CEIL(ptr, align) \
>> - RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align)
>- 1), align)
>> +#define RTE_PTR_ALIGN_CEIL(ptr, align) \
>> + __extension__({ \
>> + typeof(ptr) _pc = (ptr); \
>> + typeof(align) _ac = (align); \
>> +
> RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac -
>1), \
>> + _ac); \
>> + })
>>
>> /**
>> * Macro to align a value to a given power-of-two. The resultant value
>> @@ -303,8 +308,12 @@ static void
>__attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>> * than the first parameter. Second parameter must be a power-of-
>two
>> * value.
>> */
>> -#define RTE_ALIGN_CEIL(val, align) \
>> - RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
>> +#define RTE_ALIGN_CEIL(val, align) \
>> + __extension__({ \
>> + typeof(val) _vc = (val); \
>> + typeof(val) _ac = (typeof(val))(align); \
>> + RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
>> + })
>>
>> /**
>> * Macro to align a pointer to a given power-of-two. The resultant
>> diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
>> index 42652f9cce..863e56170b 100644
>> --- a/lib/ethdev/rte_eth_ctrl.h
>> +++ b/lib/ethdev/rte_eth_ctrl.h
>> @@ -431,8 +431,9 @@ enum rte_fdir_mode {
>> };
>>
>> #define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
>> -#define RTE_FLOW_MASK_ARRAY_SIZE \
>> - (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
>> +#define RTE_FLOW_MASK_ARRAY_SIZE \
>> + (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1),
>UINT64_BIT) / \
>> + UINT64_BIT)
>>
>> /**
>> * A structure used to get the information of flow director filter.
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
2021-05-11 8:44 ` Pavan Nikhilesh Bhagavatula
@ 2021-05-12 1:13 ` Wang, Haiyue
0 siblings, 0 replies; 19+ messages in thread
From: Wang, Haiyue @ 2021-05-12 1:13 UTC (permalink / raw)
To: Pavan Nikhilesh Bhagavatula, david.marchand, Matan Azrad,
Shahaf Shuler, Viacheslav Ovsiienko, Jiawen Wu, Jian Wang,
Chautru, Nicolas, Thomas Monjalon, Yigit, Ferruh,
Andrew Rybchenko
Cc: dev, stable
> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Sent: Tuesday, May 11, 2021 16:45
> To: Wang, Haiyue <haiyue.wang@intel.com>; david.marchand@redhat.com; Matan Azrad <matan@nvidia.com>;
> Shahaf Shuler <shahafs@nvidia.com>; Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Jiawen Wu
> <jiawenwu@trustnetic.com>; Jian Wang <jianwang@trustnetic.com>; Chautru, Nicolas
> <nicolas.chautru@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros
>
> >> -----Original Message-----
> >> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> >> Sent: Tuesday, May 11, 2021 03:40
> >> To: david.marchand@redhat.com; Wang, Haiyue
> ><haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
> >> Shahaf Shuler <shahafs@nvidia.com>; Viacheslav Ovsiienko
> ><viacheslavo@nvidia.com>; Jiawen Wu
> >> <jiawenwu@trustnetic.com>; Jian Wang <jianwang@trustnetic.com>;
> >Chautru, Nicolas
> >> <nicolas.chautru@intel.com>; Thomas Monjalon
> ><thomas@monjalon.net>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; Andrew Rybchenko
> ><andrew.rybchenko@oktetlabs.ru>
> >> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@marvell.com>;
> >stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align
> >macros
> >>
> >> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>
> >> Avoid expanding parameters inside RTE_*_ALIGN macros.
> >> Update common_autotest to detect macro side effects.
> >> Workaround static arrays relying on RTE_ALIGN macros.
> >>
> >> Fixes: af75078fece3 ("first public release")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >> ---
> >> app/test/test_common.c | 6 ++++++
> >> drivers/net/e1000/e1000_ethdev.h | 7 ++++---
> >> drivers/net/ixgbe/ixgbe_ethdev.h | 6 ++++--
> >> drivers/net/mlx5/mlx5_rxtx_vec.h | 7 ++++---
> >> drivers/net/txgbe/txgbe_ethdev.h | 6 ++++--
> >> examples/bbdev_app/main.c | 2 +-
> >> lib/eal/include/rte_common.h | 17 +++++++++++++----
> >> lib/ethdev/rte_eth_ctrl.h | 5 +++--
> >> 8 files changed, 39 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/app/test/test_common.c b/app/test/test_common.c
> >> index 0dbb87e741..9efe3b10f9 100644
> >> --- a/app/test/test_common.c
> >> +++ b/app/test/test_common.c
> >> @@ -69,6 +69,12 @@ test_macros(int __rte_unused unused_parm)
> >> TEST_SIDE_EFFECT_2(RTE_PTR_ADD, void *, size_t);
> >> TEST_SIDE_EFFECT_2(RTE_PTR_DIFF, void *, void *);
> >> TEST_SIDE_EFFECT_2(RTE_PTR_SUB, void *, size_t);
> >> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
> >> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t);
> >> + TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
> >> + TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int);
> >> + TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int, unsigned
> >int);
> >> + TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int,
> >unsigned int);
> >> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int,
> >unsigned int);
> >> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int,
> >unsigned int);
> >> TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int,
> >unsigned int);
> >> diff --git a/drivers/net/e1000/e1000_ethdev.h
> >b/drivers/net/e1000/e1000_ethdev.h
> >> index 3b4d9c3ee6..155d825d89 100644
> >> --- a/drivers/net/e1000/e1000_ethdev.h
> >> +++ b/drivers/net/e1000/e1000_ethdev.h
> >> @@ -332,9 +332,10 @@ struct igb_eth_syn_filter_ele {
> >> };
> >>
> >> #define IGB_FLEX_FILTER_MAXLEN 128 /**< bytes to use in flex
> >filter. */
> >> -#define IGB_FLEX_FILTER_MASK_SIZE \
> >> - (RTE_ALIGN(IGB_FLEX_FILTER_MAXLEN, CHAR_BIT) /
> >CHAR_BIT)
> >> - /**< mask bytes in flex filter. */
> >> +#define IGB_FLEX_FILTER_MASK_SIZE \
> >> + (RTE_ALIGN_FLOOR(IGB_FLEX_FILTER_MAXLEN + (CHAR_BIT -
> >1), CHAR_BIT) / \
> >> + CHAR_BIT)
> >> +/**< mask bytes in flex filter. */
> >>
> >
> >Since:
> >RTE_ALIGN --> RTE_ALIGN_CEIL(val, align) --> RTE_ALIGN_FLOOR(...)
> >
> >Why only change 'RTE_ALIGN_CEIL', but then call 'RTE_ALIGN_FLOOR'
> >directly ?
> >Sorry, can't get the point. : - (
> >
>
> RTE_ALIGN_CEIL is prone to macro side effects since it uses align twice in the macro.
> To fix it RTE_ALIGN_CEIL first expands the val and align values to temporary values
> and then calls ALIGN_FLOOR.
> Due to this we can no longer use ALIGN/ALIGN_CEIL in static array declarations
>
> Example:
> In file included from ../lib/eal/x86/include/rte_atomic.h:13,
> from ../examples/bbdev_app/main.c:21:
> ../lib/eal/include/rte_common.h:312:15: error: braced-group within expression allowed only inside a
> function
> __extension__({ \
> ^
> ../examples/bbdev_app/main.c:48:18: note: in expansion of macro 'RTE_ALIGN_CEIL'
> #define NCB (3 * RTE_ALIGN_CEIL(K + 4, 32))
> ^~~~~~~~~~~~~~
> ../examples/bbdev_app/main.c:145:23: note: in expansion of macro 'NCB'
> uint8_t llr_temp_buf[NCB];
>
> To workaround this I had to call RTE_ALIGN_FLOOR for known constants directly.
>
Clear now, thanks.
For Intel PMDs,
Acked-by: Haiyue Wang <haiyue.wang@intel.com>
BTW, I'm wondering that we can explicitly define the macro with type style
like RTE_ALIGN_CEIL_TYPE, so that people can notice the side effects directly,
and choose the right ALIGN macro.
And can still use ALIGN/ALIGN_CEIL in static array declarations, which is
very simple.
Just my thinking. ;-)
> >Why not change 'RTE_ALIGN' if it has issue ?
> >
> >>
> >> diff --git a/lib/eal/include/rte_common.h
> >b/lib/eal/include/rte_common.h
> >> index a142596587..6acd067b5c 100644
> >> --- a/lib/eal/include/rte_common.h
> >> +++ b/lib/eal/include/rte_common.h
> >> @@ -294,8 +294,13 @@ static void
> >__attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >> * point to an address no lower than the first parameter. Second
> >parameter
> >> * must be a power-of-two value.
> >> */
> >> -#define RTE_PTR_ALIGN_CEIL(ptr, align) \
> >> - RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(ptr, (align)
> >- 1), align)
> >> +#define RTE_PTR_ALIGN_CEIL(ptr, align) \
> >> + __extension__({ \
> >> + typeof(ptr) _pc = (ptr); \
> >> + typeof(align) _ac = (align); \
> >> +
> > RTE_PTR_ALIGN_FLOOR((typeof(ptr))RTE_PTR_ADD(_pc, _ac -
> >1), \
> >> + _ac); \
> >> + })
> >>
> >> /**
> >> * Macro to align a value to a given power-of-two. The resultant value
> >> @@ -303,8 +308,12 @@ static void
> >__attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >> * than the first parameter. Second parameter must be a power-of-
> >two
> >> * value.
> >> */
> >> -#define RTE_ALIGN_CEIL(val, align) \
> >> - RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
> >> +#define RTE_ALIGN_CEIL(val, align) \
> >> + __extension__({ \
> >> + typeof(val) _vc = (val); \
> >> + typeof(val) _ac = (typeof(val))(align); \
> >> + RTE_ALIGN_FLOOR((_vc + _ac - 1), _ac); \
> >> + })
> >>
> >> /**
> >> * Macro to align a pointer to a given power-of-two. The resultant
> >> diff --git a/lib/ethdev/rte_eth_ctrl.h b/lib/ethdev/rte_eth_ctrl.h
> >> index 42652f9cce..863e56170b 100644
> >> --- a/lib/ethdev/rte_eth_ctrl.h
> >> +++ b/lib/ethdev/rte_eth_ctrl.h
> >> @@ -431,8 +431,9 @@ enum rte_fdir_mode {
> >> };
> >>
> >> #define UINT64_BIT (CHAR_BIT * sizeof(uint64_t))
> >> -#define RTE_FLOW_MASK_ARRAY_SIZE \
> >> - (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
> >> +#define RTE_FLOW_MASK_ARRAY_SIZE \
> >> + (RTE_ALIGN_FLOOR(RTE_ETH_FLOW_MAX + (UINT64_BIT - 1),
> >UINT64_BIT) / \
> >> + UINT64_BIT)
> >>
> >> /**
> >> * A structure used to get the information of flow director filter.
> >> --
> >> 2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] eal: avoid issues in macro expansion of alignment
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
@ 2023-07-03 18:55 ` Stephen Hemminger
2023-07-03 23:23 ` [PATCH v2] " Stephen Hemminger
2023-07-04 16:01 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger
2 siblings, 1 reply; 19+ messages in thread
From: Stephen Hemminger @ 2023-07-03 18:55 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, pbhagavatula, Keith Wiles
RTE_ALIGN_MUL_NEAR is a macro so the cycle arguement could
get evaluated twice causing some potential skew. Fix by
computing value once.
Suggested by patch to fix side effects.
Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
Cc: pbhagavatula@marvell.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/eal/common/eal_common_timer.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/lib/eal/common/eal_common_timer.c b/lib/eal/common/eal_common_timer.c
index 5686a5102b66..05614b0503cf 100644
--- a/lib/eal/common/eal_common_timer.c
+++ b/lib/eal/common/eal_common_timer.c
@@ -42,10 +42,14 @@ estimate_tsc_freq(void)
RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
" - clock timings may be less accurate.\n");
/* assume that the rte_delay_us_sleep() will sleep for 1 second */
- uint64_t start = rte_rdtsc();
+ uint64_t start, elapsed;
+
+ start = rte_rdtsc();
rte_delay_us_sleep(US_PER_S);
+ elapsed = rte_rdtsc() - start;
+
/* Round up to 10Mhz. 1E7 ~ 10Mhz */
- return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
+ return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
}
void
--
2.39.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] eal: avoid issues in macro expansion of alignment
2023-07-03 18:55 ` [PATCH] eal: avoid issues in macro expansion of alignment Stephen Hemminger
@ 2023-07-03 23:23 ` Stephen Hemminger
2023-07-04 8:43 ` Morten Brørup
0 siblings, 1 reply; 19+ messages in thread
From: Stephen Hemminger @ 2023-07-03 23:23 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, pbhagavatula
RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
get evaluated twice causing some potential skew. Fix by
computing value once.
Suggested by patch to fix side effects.
Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
Cc: pbhagavatula@marvell.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - fix spelling error in commit message
lib/eal/common/eal_common_timer.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/lib/eal/common/eal_common_timer.c b/lib/eal/common/eal_common_timer.c
index 5686a5102b66..05614b0503cf 100644
--- a/lib/eal/common/eal_common_timer.c
+++ b/lib/eal/common/eal_common_timer.c
@@ -42,10 +42,14 @@ estimate_tsc_freq(void)
RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
" - clock timings may be less accurate.\n");
/* assume that the rte_delay_us_sleep() will sleep for 1 second */
- uint64_t start = rte_rdtsc();
+ uint64_t start, elapsed;
+
+ start = rte_rdtsc();
rte_delay_us_sleep(US_PER_S);
+ elapsed = rte_rdtsc() - start;
+
/* Round up to 10Mhz. 1E7 ~ 10Mhz */
- return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
+ return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
}
void
--
2.39.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] eal: avoid issues in macro expansion of alignment
2023-07-03 23:23 ` [PATCH v2] " Stephen Hemminger
@ 2023-07-04 8:43 ` Morten Brørup
2023-07-04 16:00 ` Stephen Hemminger
0 siblings, 1 reply; 19+ messages in thread
From: Morten Brørup @ 2023-07-04 8:43 UTC (permalink / raw)
To: Stephen Hemminger, dev; +Cc: pbhagavatula
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 4 July 2023 01.24
>
> RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
> get evaluated twice causing some potential skew. Fix by
> computing value once.
>
> Suggested by patch to fix side effects.
>
> Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
> Cc: pbhagavatula@marvell.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - fix spelling error in commit message
>
> lib/eal/common/eal_common_timer.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_timer.c
> b/lib/eal/common/eal_common_timer.c
> index 5686a5102b66..05614b0503cf 100644
> --- a/lib/eal/common/eal_common_timer.c
> +++ b/lib/eal/common/eal_common_timer.c
> @@ -42,10 +42,14 @@ estimate_tsc_freq(void)
> RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
> " - clock timings may be less accurate.\n");
> /* assume that the rte_delay_us_sleep() will sleep for 1 second */
> - uint64_t start = rte_rdtsc();
> + uint64_t start, elapsed;
> +
> + start = rte_rdtsc();
> rte_delay_us_sleep(US_PER_S);
> + elapsed = rte_rdtsc() - start;
> +
> /* Round up to 10Mhz. 1E7 ~ 10Mhz */
> - return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> + return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
> }
>
> void
> --
> 2.39.2
Please fix the RTE_ALIGN_MUL_NEAR() macro instead. It already uses temporary variables with typeof() anyway.
Other macros might have similar behavior of using their parameters more than once, and could be improved too.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] eal: avoid issues in macro expansion of alignment
2023-07-04 8:43 ` Morten Brørup
@ 2023-07-04 16:00 ` Stephen Hemminger
2023-07-04 22:16 ` Morten Brørup
0 siblings, 1 reply; 19+ messages in thread
From: Stephen Hemminger @ 2023-07-04 16:00 UTC (permalink / raw)
To: Morten Brørup; +Cc: dev, pbhagavatula
On Tue, 4 Jul 2023 10:43:40 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, 4 July 2023 01.24
> >
> > RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
> > get evaluated twice causing some potential skew. Fix by
> > computing value once.
> >
> > Suggested by patch to fix side effects.
> >
> > Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
> > Cc: pbhagavatula@marvell.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > v2 - fix spelling error in commit message
> >
> > lib/eal/common/eal_common_timer.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/eal/common/eal_common_timer.c
> > b/lib/eal/common/eal_common_timer.c
> > index 5686a5102b66..05614b0503cf 100644
> > --- a/lib/eal/common/eal_common_timer.c
> > +++ b/lib/eal/common/eal_common_timer.c
> > @@ -42,10 +42,14 @@ estimate_tsc_freq(void)
> > RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
> > " - clock timings may be less accurate.\n");
> > /* assume that the rte_delay_us_sleep() will sleep for 1 second */
> > - uint64_t start = rte_rdtsc();
> > + uint64_t start, elapsed;
> > +
> > + start = rte_rdtsc();
> > rte_delay_us_sleep(US_PER_S);
> > + elapsed = rte_rdtsc() - start;
> > +
> > /* Round up to 10Mhz. 1E7 ~ 10Mhz */
> > - return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> > + return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
> > }
> >
> > void
> > --
> > 2.39.2
>
> Please fix the RTE_ALIGN_MUL_NEAR() macro instead. It already uses temporary variables with typeof() anyway.
>
> Other macros might have similar behavior of using their parameters more than once, and could be improved too.
>
It is already fixed, so this patch can be dropped.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
2023-07-03 18:55 ` [PATCH] eal: avoid issues in macro expansion of alignment Stephen Hemminger
@ 2023-07-04 16:01 ` Stephen Hemminger
2 siblings, 0 replies; 19+ messages in thread
From: Stephen Hemminger @ 2023-07-04 16:01 UTC (permalink / raw)
To: pbhagavatula; +Cc: david.marchand, dev, stable, Tyler Retzlaff
On Tue, 11 May 2021 01:10:06 +0530
<pbhagavatula@marvell.com> wrote:
> Avoid expanding parameters inside the macro multiple times.
> For example:
> RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> Here rte_rdtsc() call is expanded multiple times in the macro
> causing it to return different values that leads to unintended
> side effects.
This was already fixed.
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] eal: avoid issues in macro expansion of alignment
2023-07-04 16:00 ` Stephen Hemminger
@ 2023-07-04 22:16 ` Morten Brørup
2023-07-14 16:37 ` Stephen Hemminger
0 siblings, 1 reply; 19+ messages in thread
From: Morten Brørup @ 2023-07-04 22:16 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, pbhagavatula
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 4 July 2023 18.01
>
> On Tue, 4 Jul 2023 10:43:40 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Tuesday, 4 July 2023 01.24
> > >
> > > RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
> > > get evaluated twice causing some potential skew. Fix by
> > > computing value once.
> > >
> > > Suggested by patch to fix side effects.
> > >
> > > Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
> > > Cc: pbhagavatula@marvell.com
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > > v2 - fix spelling error in commit message
> > >
> > > lib/eal/common/eal_common_timer.c | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/eal/common/eal_common_timer.c
> > > b/lib/eal/common/eal_common_timer.c
> > > index 5686a5102b66..05614b0503cf 100644
> > > --- a/lib/eal/common/eal_common_timer.c
> > > +++ b/lib/eal/common/eal_common_timer.c
> > > @@ -42,10 +42,14 @@ estimate_tsc_freq(void)
> > > RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
> > > " - clock timings may be less accurate.\n");
> > > /* assume that the rte_delay_us_sleep() will sleep for 1 second */
> > > - uint64_t start = rte_rdtsc();
> > > + uint64_t start, elapsed;
> > > +
> > > + start = rte_rdtsc();
> > > rte_delay_us_sleep(US_PER_S);
> > > + elapsed = rte_rdtsc() - start;
> > > +
> > > /* Round up to 10Mhz. 1E7 ~ 10Mhz */
> > > - return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> > > + return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
> > > }
> > >
> > > void
> > > --
> > > 2.39.2
> >
> > Please fix the RTE_ALIGN_MUL_NEAR() macro instead. It already uses
> temporary variables with typeof() anyway.
> >
> > Other macros might have similar behavior of using their parameters
> more than once, and could be improved too.
> >
>
> It is already fixed, so this patch can be dropped.
The macro is not fixed in 23.07-rc2 [1], but evaluates the "v" parameter four times; first two times to calculate respectively "ceil" and "floor", and then two times in the trigraph to determine which way to round. The "mul" parameter is evaluated twice by the macro.
#define RTE_ALIGN_MUL_NEAR(v, mul) \
({ \
typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \
typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \
(ceil - (v)) > ((v) - floor) ? floor : ceil; \
})
[1]: https://elixir.bootlin.com/dpdk/v23.07-rc2/source/lib/eal/include/rte_common.h#L388
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] eal: avoid issues in macro expansion of alignment
2023-07-04 22:16 ` Morten Brørup
@ 2023-07-14 16:37 ` Stephen Hemminger
0 siblings, 0 replies; 19+ messages in thread
From: Stephen Hemminger @ 2023-07-14 16:37 UTC (permalink / raw)
To: Morten Brørup; +Cc: dev, pbhagavatula
On Wed, 5 Jul 2023 00:16:50 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, 4 July 2023 18.01
> >
> > On Tue, 4 Jul 2023 10:43:40 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, 4 July 2023 01.24
> > > >
> > > > RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
> > > > get evaluated twice causing some potential skew. Fix by
> > > > computing value once.
> > > >
> > > > Suggested by patch to fix side effects.
> > > >
> > > > Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
> > > > Cc: pbhagavatula@marvell.com
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > > v2 - fix spelling error in commit message
> > > >
> > > > lib/eal/common/eal_common_timer.c | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/eal/common/eal_common_timer.c
> > > > b/lib/eal/common/eal_common_timer.c
> > > > index 5686a5102b66..05614b0503cf 100644
> > > > --- a/lib/eal/common/eal_common_timer.c
> > > > +++ b/lib/eal/common/eal_common_timer.c
> > > > @@ -42,10 +42,14 @@ estimate_tsc_freq(void)
> > > > RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
> > > > " - clock timings may be less accurate.\n");
> > > > /* assume that the rte_delay_us_sleep() will sleep for 1 second */
> > > > - uint64_t start = rte_rdtsc();
> > > > + uint64_t start, elapsed;
> > > > +
> > > > + start = rte_rdtsc();
> > > > rte_delay_us_sleep(US_PER_S);
> > > > + elapsed = rte_rdtsc() - start;
> > > > +
> > > > /* Round up to 10Mhz. 1E7 ~ 10Mhz */
> > > > - return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> > > > + return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
> > > > }
> > > >
> > > > void
> > > > --
> > > > 2.39.2
> > >
> > > Please fix the RTE_ALIGN_MUL_NEAR() macro instead. It already uses
> > temporary variables with typeof() anyway.
> > >
> > > Other macros might have similar behavior of using their parameters
> > more than once, and could be improved too.
> > >
> >
> > It is already fixed, so this patch can be dropped.
>
> The macro is not fixed in 23.07-rc2 [1], but evaluates the "v" parameter four times; first two times to calculate respectively "ceil" and "floor", and then two times in the trigraph to determine which way to round. The "mul" parameter is evaluated twice by the macro.
>
> #define RTE_ALIGN_MUL_NEAR(v, mul) \
> ({ \
> typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \
> typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \
> (ceil - (v)) > ((v) - floor) ? floor : ceil; \
> })
>
> [1]: https://elixir.bootlin.com/dpdk/v23.07-rc2/source/lib/eal/include/rte_common.h#L388
>
Still working on sorting this out. Discovered this new issue.
If I fix RTE_ALIGN_CEIL to only evaluate arguments once.
-#define RTE_ALIGN_CEIL(val, align) \
- RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
+#define RTE_ALIGN_CEIL(val, align) \
+ __extension__ ({ \
+ uintptr_t _align = align; \
+ RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (_align) - 1)), _align); \
+ })
Then compiler won't allow it to be used in initialization of globals like this.
In file included from ../lib/telemetry/rte_telemetry.h:14,
from ../lib/ethdev/rte_ethdev_telemetry.c:9:
../lib/eal/include/rte_common.h:350:23: error: braced-group within expression allowed only inside a function
350 | __extension__ ({ \
| ^
../lib/eal/include/rte_common.h:371:31: note: in expansion of macro ‘RTE_ALIGN_CEIL’
371 | #define RTE_ALIGN(val, align) RTE_ALIGN_CEIL(val, align)
| ^~~~~~~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:435:10: note: in expansion of macro ‘RTE_ALIGN’
435 | (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
| ^~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:452:34: note: in expansion of macro ‘RTE_FLOW_MASK_ARRAY_SIZE’
452 | uint64_t flow_types_mask[RTE_FLOW_MASK_ARRAY_SIZE];
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-07-14 16:37 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-09 17:18 [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-09 19:39 ` Stephen Hemminger
2021-05-10 9:50 ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2021-05-10 13:34 ` David Marchand
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 19:40 ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-11 2:12 ` Wang, Haiyue
2021-05-11 8:44 ` Pavan Nikhilesh Bhagavatula
2021-05-12 1:13 ` Wang, Haiyue
2023-07-03 18:55 ` [PATCH] eal: avoid issues in macro expansion of alignment Stephen Hemminger
2023-07-03 23:23 ` [PATCH v2] " Stephen Hemminger
2023-07-04 8:43 ` Morten Brørup
2023-07-04 16:00 ` Stephen Hemminger
2023-07-04 22:16 ` Morten Brørup
2023-07-14 16:37 ` Stephen Hemminger
2023-07-04 16:01 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger
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).