* [dpdk-stable] [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-stable] [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula 2021-05-10 14:02 ` [dpdk-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula 0 siblings, 2 replies; 13+ 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] 13+ messages in thread
* [dpdk-stable] [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros 2021-05-09 17:18 [dpdk-stable] [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-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula 1 sibling, 1 reply; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros 2021-05-09 17:18 ` [dpdk-stable] [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-stable] [EXT] " Pavan Nikhilesh Bhagavatula 0 siblings, 1 reply; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [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; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros 2021-05-10 9:50 ` [dpdk-stable] [EXT] " Pavan Nikhilesh Bhagavatula @ 2021-05-10 13:34 ` David Marchand 0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* [dpdk-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros 2021-05-09 17:18 [dpdk-stable] [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-09 17:18 ` [dpdk-stable] [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-stable] [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula 2021-05-10 19:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula 1 sibling, 2 replies; 13+ 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] 13+ messages in thread
* [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros 2021-05-10 14:02 ` [dpdk-stable] [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-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula 1 sibling, 0 replies; 13+ 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] 13+ messages in thread
* [dpdk-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros 2021-05-10 14:02 ` [dpdk-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-10 14:02 ` [dpdk-stable] [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-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula 2023-07-04 16:01 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger 1 sibling, 2 replies; 13+ 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] 13+ messages in thread
* [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros 2021-05-10 19:40 ` [dpdk-stable] [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-04 16:01 ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger 1 sibling, 1 reply; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros 2021-05-10 19:40 ` [dpdk-stable] [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; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [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; 13+ 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] 13+ messages in thread
* Re: [dpdk-stable] [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; 13+ 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] 13+ messages in thread
* Re: [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros 2021-05-10 19:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-10 19:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula @ 2023-07-04 16:01 ` Stephen Hemminger 1 sibling, 0 replies; 13+ 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] 13+ messages in thread
end of thread, other threads:[~2023-07-04 16:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-09 17:18 [dpdk-stable] [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-09 17:18 ` [dpdk-stable] [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-stable] [EXT] " Pavan Nikhilesh Bhagavatula 2021-05-10 13:34 ` David Marchand 2021-05-10 14:02 ` [dpdk-stable] [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-10 14:02 ` [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula 2021-05-10 19:40 ` [dpdk-stable] [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula 2021-05-10 19:40 ` [dpdk-stable] [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-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).