diff mbox series

[v3,2/2] eal: fix side effects in ptr align macros

Message ID 20210510194008.403-2-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v3,1/2] eal: fix side effects in align mul macros |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/github-robot success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Pavan Nikhilesh Bhagavatula May 10, 2021, 7:40 p.m. UTC 
  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

Wang, Haiyue May 11, 2021, 2:12 a.m. UTC | #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. : - (

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
Pavan Nikhilesh Bhagavatula May 11, 2021, 8:44 a.m. UTC | #2 
>> -----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
Wang, Haiyue May 12, 2021, 1:13 a.m. UTC | #3 
> -----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
diff mbox series

Patch

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.