Found an issue while using RTE_ALIGN_MUL_NEAR with an expression, like as passed in estimate_tsc_freq(). RTE_ALIGN_MUL_FLOOR resulted in unexpected value in the above function as division has more precedence over substraction. Fixes: 5120203d753f ("eal: add macros to align value to multiple") Cc: stable@dpdk.org Signed-off-by: Harman Kalra <hkalra@marvell.com> --- lib/librte_eal/include/rte_common.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h index 0843ce69e..0d834001c 100644 --- a/lib/librte_eal/include/rte_common.h +++ b/lib/librte_eal/include/rte_common.h @@ -295,7 +295,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) * than the first parameter. */ #define RTE_ALIGN_MUL_CEIL(v, mul) \ - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) /** * Macro to align a value to the multiple of given value. The resultant @@ -303,7 +303,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) * than the first parameter. */ #define RTE_ALIGN_MUL_FLOOR(v, mul) \ - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) + (((v) / ((typeof(v))(mul))) * (typeof(v))(mul)) /** * Macro to align value to the nearest multiple of the given value. @@ -314,7 +314,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) ({ \ typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \ typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \ - (ceil - v) > (v - floor) ? floor : ceil; \ + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ }) /** -- 2.18.0
09/06/2020 21:17, Harman Kalra: > Found an issue while using RTE_ALIGN_MUL_NEAR with an > expression, like as passed in estimate_tsc_freq(). > RTE_ALIGN_MUL_FLOOR resulted in unexpected value in the > above function as division has more precedence over > substraction. The only change I see is adding parenthesis around v. Am I right? > #define RTE_ALIGN_MUL_CEIL(v, mul) \ > - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) [...] > #define RTE_ALIGN_MUL_FLOOR(v, mul) \ > - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) > + (((v) / ((typeof(v))(mul))) * (typeof(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; \ > + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ > })
On Wed, Jun 24, 2020 at 10:13:18AM +0200, Thomas Monjalon wrote: > External Email > > ---------------------------------------------------------------------- > 09/06/2020 21:17, Harman Kalra: > > Found an issue while using RTE_ALIGN_MUL_NEAR with an > > expression, like as passed in estimate_tsc_freq(). > > RTE_ALIGN_MUL_FLOOR resulted in unexpected value in the > > above function as division has more precedence over > > substraction. > > The only change I see is adding parenthesis around v. > Am I right? Yes, parathesis are required if an expression is passed. > > > #define RTE_ALIGN_MUL_CEIL(v, mul) \ > > - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > > + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > [...] > > #define RTE_ALIGN_MUL_FLOOR(v, mul) \ > > - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) > > + (((v) / ((typeof(v))(mul))) * (typeof(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; \ > > + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ > > }) > > > >
24/06/2020 10:24, Harman Kalra: > On Wed, Jun 24, 2020 at 10:13:18AM +0200, Thomas Monjalon wrote: > > 09/06/2020 21:17, Harman Kalra: > > > Found an issue while using RTE_ALIGN_MUL_NEAR with an > > > expression, like as passed in estimate_tsc_freq(). > > > RTE_ALIGN_MUL_FLOOR resulted in unexpected value in the > > > above function as division has more precedence over > > > substraction. > > > > The only change I see is adding parenthesis around v. > > Am I right? > > Yes, parathesis are required if an expression is passed. I think the commit log needs to be updated. I don't see the relation between "division has more precedence over substraction" and "parathesis are required if an expression is passed" > > > #define RTE_ALIGN_MUL_CEIL(v, mul) \ > > > - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > > > + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > > [...] > > > #define RTE_ALIGN_MUL_FLOOR(v, mul) \ > > > - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) > > > + (((v) / ((typeof(v))(mul))) * (typeof(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; \ > > > + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ > > > }) > > > > > > > > >
On Wed, Jun 24, 2020 at 10:30:18AM +0200, Thomas Monjalon wrote: > 24/06/2020 10:24, Harman Kalra: > > On Wed, Jun 24, 2020 at 10:13:18AM +0200, Thomas Monjalon wrote: > > > 09/06/2020 21:17, Harman Kalra: > > > > Found an issue while using RTE_ALIGN_MUL_NEAR with an > > > > expression, like as passed in estimate_tsc_freq(). > > > > RTE_ALIGN_MUL_FLOOR resulted in unexpected value in the > > > > above function as division has more precedence over > > > > substraction. > > > > > > The only change I see is adding parenthesis around v. > > > Am I right? > > > > Yes, parathesis are required if an expression is passed. > > I think the commit log needs to be updated. > I don't see the relation between > "division has more precedence over substraction" > and > "parathesis are required if an expression is passed" By "division has more precedence over substraction", I tried to highlight the issue which resulted in unexpected value, but yes it is sounding confusing. I will reword the commit message and send V2. > > > > > > #define RTE_ALIGN_MUL_CEIL(v, mul) \ > > > > - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > > > > + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) > > > [...] > > > > #define RTE_ALIGN_MUL_FLOOR(v, mul) \ > > > > - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) > > > > + (((v) / ((typeof(v))(mul))) * (typeof(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; \ > > > > + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ > > > > }) > > > > > > > > > > > > > > > > > > >
Found an issue while using RTE_ALIGN_MUL_NEAR with an expression, like as passed in estimate_tsc_freq(). RTE_ALIGN_MUL_FLOOR resulted in unexpected value as parathesis are required to evaluate an expression. Fixes: 5120203d753f ("eal: add macros to align value to multiple") Cc: stable@dpdk.org Signed-off-by: Harman Kalra <hkalra@marvell.com> --- *V2: improved commit message. lib/librte_eal/include/rte_common.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h index 0843ce69e..0d834001c 100644 --- a/lib/librte_eal/include/rte_common.h +++ b/lib/librte_eal/include/rte_common.h @@ -295,7 +295,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) * than the first parameter. */ #define RTE_ALIGN_MUL_CEIL(v, mul) \ - (((v + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) + ((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul)) /** * Macro to align a value to the multiple of given value. The resultant @@ -303,7 +303,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) * than the first parameter. */ #define RTE_ALIGN_MUL_FLOOR(v, mul) \ - ((v / ((typeof(v))(mul))) * (typeof(v))(mul)) + (((v) / ((typeof(v))(mul))) * (typeof(v))(mul)) /** * Macro to align value to the nearest multiple of the given value. @@ -314,7 +314,7 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) ({ \ typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul); \ typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul); \ - (ceil - v) > (v - floor) ? floor : ceil; \ + (ceil - (v)) > ((v) - floor) ? floor : ceil; \ }) /** -- 2.18.0
24/06/2020 12:20, Harman Kalra:
> Found an issue while using RTE_ALIGN_MUL_NEAR with an
> expression, like as passed in estimate_tsc_freq().
> RTE_ALIGN_MUL_FLOOR resulted in unexpected value as
> parathesis are required to evaluate an expression.
>
> Fixes: 5120203d753f ("eal: add macros to align value to multiple")
> Cc: stable@dpdk.org
>
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
Applied, thanks