[dpdk-stable] [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros

[dpdk-stable] [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros

[pbhagavatula]

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

[dpdk-stable] [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros

[pbhagavatula]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros

[Stephen Hemminger]

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?

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros

[Pavan Nikhilesh Bhagavatula]

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

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros

[David Marchand]

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

[dpdk-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros

[pbhagavatula]

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

[dpdk-stable] [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros

[pbhagavatula]

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

[dpdk-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros

[pbhagavatula]

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

[dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros

[pbhagavatula]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros

[Wang, Haiyue]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros

[Pavan Nikhilesh Bhagavatula]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros

[Wang, Haiyue]

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

Re: [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros

[Stephen Hemminger]

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.