[dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Tyler Retzlaff]

Avoid expanding v and mul parameters multiple times in the macro. based
on usage of the macro it seems like side effects were not intended.

For example:
  ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/include/rte_common.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
index 1b630baf1..640befee2 100644
--- a/lib/librte_eal/include/rte_common.h
+++ b/lib/librte_eal/include/rte_common.h
@@ -345,9 +345,11 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  */
 #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;	\
+		typeof(v) _v = (v);				\
+		typeof(v) _m = (mul);				\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m);	\
+		(ceil - (_v)) > ((_v) - floor) ? floor : ceil;	\
 	})
 
 /**
-- 
2.30.0.vfs.0.2

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Ranjit Menon]

On 3/11/2021 1:08 PM, Tyler Retzlaff wrote:
> Avoid expanding v and mul parameters multiple times in the macro. based
> on usage of the macro it seems like side effects were not intended.
>
> For example:
>    ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   lib/librte_eal/include/rte_common.h | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> index 1b630baf1..640befee2 100644
> --- a/lib/librte_eal/include/rte_common.h
> +++ b/lib/librte_eal/include/rte_common.h
> @@ -345,9 +345,11 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>    */
>   #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;	\
> +		typeof(v) _v = (v);				\
> +		typeof(v) _m = (mul);				\
For consistency sake, can this variable be changed to '_mul'?
> +		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);	\
> +		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m);	\
> +		(ceil - (_v)) > ((_v) - floor) ? floor : ceil;	\
>   	})
>   
>   /**


ranjit m.

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Tyler Retzlaff]

On Thu, Mar 11, 2021 at 04:40:58PM -0800, Ranjit Menon wrote:
> 
> On 3/11/2021 1:08 PM, Tyler Retzlaff wrote:
> >diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> >index 1b630baf1..640befee2 100644
> >--- a/lib/librte_eal/include/rte_common.h
> >+++ b/lib/librte_eal/include/rte_common.h
> >@@ -345,9 +345,11 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >   */
> >  #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;	\
> >+		typeof(v) _v = (v);				\
> >+		typeof(v) _m = (mul);				\
> For consistency sake, can this variable be changed to '_mul'?

sure, i can do that. it will hit another tab stop below though.
i'll submit a v2 with the requested change.

> >+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);	\
> >+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m);	\
> >+		(ceil - (_v)) > ((_v) - floor) ? floor : ceil;	\
> >  	})
> >  /**
> 
> 
> ranjit m.

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Ranjit Menon]

On 3/11/2021 5:34 PM, Tyler Retzlaff wrote:
> On Thu, Mar 11, 2021 at 04:40:58PM -0800, Ranjit Menon wrote:
>> On 3/11/2021 1:08 PM, Tyler Retzlaff wrote:
>>> diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
>>> index 1b630baf1..640befee2 100644
>>> --- a/lib/librte_eal/include/rte_common.h
>>> +++ b/lib/librte_eal/include/rte_common.h
>>> @@ -345,9 +345,11 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>>>    */
>>>   #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;	\
>>> +		typeof(v) _v = (v);				\
>>> +		typeof(v) _m = (mul);				\
>> For consistency sake, can this variable be changed to '_mul'?
> sure, i can do that. it will hit another tab stop below though.
> i'll submit a v2 with the requested change.

Ugh! In that case, let it be. If others insist on the change, we can do it.

ranjit m.

>
>>> +		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);	\
>>> +		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m);	\
>>> +		(ceil - (_v)) > ((_v) - floor) ? floor : ceil;	\
>>>   	})
>>>   /**
>>
>> ranjit m.

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Tyler Retzlaff]

On Thu, Mar 11, 2021 at 08:41:58PM -0800, Ranjit Menon wrote:
> 
> On 3/11/2021 5:34 PM, Tyler Retzlaff wrote:
> >On Thu, Mar 11, 2021 at 04:40:58PM -0800, Ranjit Menon wrote:
> >>On 3/11/2021 1:08 PM, Tyler Retzlaff wrote:
> >>>diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
> >>>index 1b630baf1..640befee2 100644
> >>>--- a/lib/librte_eal/include/rte_common.h
> >>>+++ b/lib/librte_eal/include/rte_common.h
> >>>@@ -345,9 +345,11 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >>>   */
> >>>  #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;	\
> >>>+		typeof(v) _v = (v);				\
> >>>+		typeof(v) _m = (mul);				\
> >>For consistency sake, can this variable be changed to '_mul'?
> >sure, i can do that. it will hit another tab stop below though.
> >i'll submit a v2 with the requested change.
> 
> Ugh! In that case, let it be. If others insist on the change, we can do it.

heh, yeah painful. okay i will leave the patch as it is and wait a bit
for someone with a strong opinion.

> 
> ranjit m.
> 
> >
> >>>+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);	\

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[David Marchand]

On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Avoid expanding v and mul parameters multiple times in the macro. based
> on usage of the macro it seems like side effects were not intended.
>
> For example:
>   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''

That's the beauty of macros.
How about updating the unit tests so that this kind of issue is not
reintroduced?


Are other RTE_ALIGN_* macro affected with similar issues?
Like mul expression passed to RTE_ALIGN_MUL_FLOOR and RTE_ALIGN_MUL_CEIL.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Tyler Retzlaff]

On Fri, Mar 12, 2021 at 09:07:22AM +0100, David Marchand wrote:
> On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Avoid expanding v and mul parameters multiple times in the macro. based
> > on usage of the macro it seems like side effects were not intended.
> >
> > For example:
> >   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''
> 
> That's the beauty of macros.
> How about updating the unit tests so that this kind of issue is not
> reintroduced?

i would like to. if i can find time i will but i can't promise.

> 
> 
> Are other RTE_ALIGN_* macro affected with similar issues?
> Like mul expression passed to RTE_ALIGN_MUL_FLOOR and RTE_ALIGN_MUL_CEIL.

it is possible. i did look at a number of these macros while evaluating
something else but until i ran into this looking for side effects wasn't
on my radar.

> 
> 
> -- 
> David Marchand

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Tyler Retzlaff]

On Fri, Mar 12, 2021 at 09:07:22AM +0100, David Marchand wrote:
> On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Avoid expanding v and mul parameters multiple times in the macro. based
> > on usage of the macro it seems like side effects were not intended.
> >
> > For example:
> >   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''
> 
> That's the beauty of macros.
> How about updating the unit tests so that this kind of issue is not
> reintroduced?

i'm afraid i don't have the schedule budget to do this. i get very
little dpdk time.

do you want me to withdraw the fix?

> 
> 
> Are other RTE_ALIGN_* macro affected with similar issues?
> Like mul expression passed to RTE_ALIGN_MUL_FLOOR and RTE_ALIGN_MUL_CEIL.
> 
> 
> -- 
> David Marchand

Re: [dpdk-dev] [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[David Marchand]

On Wed, May 5, 2021 at 6:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Fri, Mar 12, 2021 at 09:07:22AM +0100, David Marchand wrote:
> > On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > Avoid expanding v and mul parameters multiple times in the macro. based
> > > on usage of the macro it seems like side effects were not intended.
> > >
> > > For example:
> > >   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);''
> >
> > That's the beauty of macros.
> > How about updating the unit tests so that this kind of issue is not
> > reintroduced?
>
> i'm afraid i don't have the schedule budget to do this. i get very
> little dpdk time.

I started to write a test earlier (see below) but I did not finish either.
We can wait until somebody has the time to investigate/finish the work.
I copied Pavan who authored RTE_ALIGN_MUL_*.

There are more macros affected than the one you fixed (see commented
lines with FIXME:), and there are probably more macros to check in
rte_common.h:

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..44770722a7 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)
@@ -28,8 +35,18 @@ test_macros(int __rte_unused unused_parm)
 #define FAIL_MACRO(x)\
        {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 +64,19 @@ 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);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *, size_t); */
+       TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int, unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int,
unsigned int); */
+       TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned int);
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int,
unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned
int, unsigned int); */
+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int,
unsigned int); */
+       return ret;
 }

 static int



-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH] eal: avoid side effects in RTE_ALIGN_MUL_NEAR(v, mul) for v and mul

[Pavan Nikhilesh Bhagavatula]

>On Wed, May 5, 2021 at 6:30 PM Tyler Retzlaff
><roretzla@linux.microsoft.com> wrote:
>>
>> On Fri, Mar 12, 2021 at 09:07:22AM +0100, David Marchand wrote:
>> > On Thu, Mar 11, 2021 at 10:08 PM Tyler Retzlaff
>> > <roretzla@linux.microsoft.com> wrote:
>> > >
>> > > Avoid expanding v and mul parameters multiple times in the
>macro. based
>> > > on usage of the macro it seems like side effects were not
>intended.
>> > >
>> > > For example:
>> > >   ``return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start,
>CYC_PER_10MHZ);''
>> >
>> > That's the beauty of macros.
>> > How about updating the unit tests so that this kind of issue is not
>> > reintroduced?
>>
>> i'm afraid i don't have the schedule budget to do this. i get very
>> little dpdk time.
>
>I started to write a test earlier (see below) but I did not finish either.
>We can wait until somebody has the time to investigate/finish the work.
>I copied Pavan who authored RTE_ALIGN_MUL_*.

Hi David,

I will send a patch to fix RTE_ALIGN_MUL_* + testcase to verify side-effect.

I tried to fix the RTE_ALIGN_* too but looks like since they are used to statically 
set array sizes it’s a bit complicated to fix them 

ex.
../lib/eal/include/rte_common.h:311:2: error: braced-group within expression allowed only inside a function
  311 |  ({                                                                     \
      |  ^
../lib/eal/include/rte_common.h:333:31: note: in expansion of macro ‘RTE_ALIGN_CEIL’
  333 | #define RTE_ALIGN(val, align) RTE_ALIGN_CEIL(val, align)
      |                               ^~~~~~~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:435:3: note: in expansion of macro ‘RTE_ALIGN’
  435 |  (RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT64_BIT)/UINT64_BIT)
      |   ^~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:452:27: note: in expansion of macro ‘RTE_FLOW_MASK_ARRAY_SIZE’
  452 |  uint64_t flow_types_mask[RTE_FLOW_MASK_ARRAY_SIZE];

	
I will send a separate patch to fix RTE_ALIGN* stuff and we can move forward based on acceptance.

Regards,
Pavan.

>
>There are more macros affected than the one you fixed (see
>commented
>lines with FIXME:), and there are probably more macros to check in
>rte_common.h:
>
>diff --git a/app/test/test_common.c b/app/test/test_common.c
>index 12bd1cad90..44770722a7 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)
>@@ -28,8 +35,18 @@ test_macros(int __rte_unused unused_parm)
> #define FAIL_MACRO(x)\
>        {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 +64,19 @@ 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);
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN, void *, size_t);
>*/
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_CEIL, void *,
>size_t); */
>+       TEST_SIDE_EFFECT_2(RTE_PTR_ALIGN_FLOOR, void *, size_t);
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN, unsigned int,
>unsigned int); */
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_CEIL, unsigned int,
>unsigned int); */
>+       TEST_SIDE_EFFECT_2(RTE_ALIGN_FLOOR, unsigned int, unsigned
>int);
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned
>int,
>unsigned int); */
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR,
>unsigned
>int, unsigned int); */
>+       /* FIXME: TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned
>int,
>unsigned int); */
>+       return ret;
> }
>
> static int
>
>
>
>--
>David Marchand