[v3,2/2] eal: fix side effects in ptr align macros
Checks
Commit Message
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(-)
Comments
> -----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
>> -----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
> -----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
@@ -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);
@@ -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
@@ -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 {
@@ -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);
@@ -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 {
@@ -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
@@ -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
@@ -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.