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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>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