Re: [PATCH v2] eal: avoid issues in macro expansion of alignment

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 5 Jul 2023 00:16:50 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, 4 July 2023 18.01
> > 
> > On Tue, 4 Jul 2023 10:43:40 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >   
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, 4 July 2023 01.24
> > > >
> > > > RTE_ALIGN_MUL_NEAR is a macro so the cycle argument could
> > > > get evaluated twice causing some potential skew.  Fix by
> > > > computing value once.
> > > >
> > > > Suggested by patch to fix side effects.
> > > >
> > > > Fixes: 5cbd14b3e5f9 ("eal: roundup TSC frequency when estimating")
> > > > Cc: pbhagavatula@marvell.com
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > > v2 - fix spelling error in commit message
> > > >
> > > >  lib/eal/common/eal_common_timer.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/eal/common/eal_common_timer.c
> > > > b/lib/eal/common/eal_common_timer.c
> > > > index 5686a5102b66..05614b0503cf 100644
> > > > --- a/lib/eal/common/eal_common_timer.c
> > > > +++ b/lib/eal/common/eal_common_timer.c
> > > > @@ -42,10 +42,14 @@ estimate_tsc_freq(void)
> > > >  	RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
> > > >  		" - clock timings may be less accurate.\n");
> > > >  	/* assume that the rte_delay_us_sleep() will sleep for 1 second */
> > > > -	uint64_t start = rte_rdtsc();
> > > > +	uint64_t start, elapsed;
> > > > +
> > > > +	start = rte_rdtsc();
> > > >  	rte_delay_us_sleep(US_PER_S);
> > > > +	elapsed = rte_rdtsc() - start;
> > > > +
> > > >  	/* Round up to 10Mhz. 1E7 ~ 10Mhz */
> > > > -	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
> > > > +	return RTE_ALIGN_MUL_NEAR(elapsed, CYC_PER_10MHZ);
> > > >  }
> > > >
> > > >  void
> > > > --
> > > > 2.39.2  
> > >
> > > Please fix the RTE_ALIGN_MUL_NEAR() macro instead. It already uses  
> > temporary variables with typeof() anyway.  
> > >
> > > Other macros might have similar behavior of using their parameters  
> > more than once, and could be improved too.  
> > >  
> > 
> > It is already fixed, so this patch can be dropped.  
> 
> The macro is not fixed in 23.07-rc2 [1], but evaluates the "v" parameter four times; first two times to calculate respectively "ceil" and "floor", and then two times in the trigraph to determine which way to round. The "mul" parameter is evaluated twice by the macro.
> 
> #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;	\
> 	})
> 
> [1]: https://elixir.bootlin.com/dpdk/v23.07-rc2/source/lib/eal/include/rte_common.h#L388
> 

Still working on sorting this out.  Discovered this new issue.

If I fix RTE_ALIGN_CEIL to only evaluate arguments once.

-#define RTE_ALIGN_CEIL(val, align) \
-	RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (align) - 1)), align)
+#define RTE_ALIGN_CEIL(val, align)			\
+	__extension__ ({				\
+		uintptr_t _align = align;		\
+		RTE_ALIGN_FLOOR(((val) + ((typeof(val)) (_align) - 1)), _align); \
+	})
 

Then compiler won't allow it to be used in initialization of globals like this.

In file included from ../lib/telemetry/rte_telemetry.h:14,
                 from ../lib/ethdev/rte_ethdev_telemetry.c:9:
../lib/eal/include/rte_common.h:350:23: error: braced-group within expression allowed only inside a function
  350 |         __extension__ ({                                \
      |                       ^
../lib/eal/include/rte_common.h:371:31: note: in expansion of macro ‘RTE_ALIGN_CEIL’
  371 | #define RTE_ALIGN(val, align) RTE_ALIGN_CEIL(val, align)
      |                               ^~~~~~~~~~~~~~
../lib/ethdev/rte_eth_ctrl.h:435:10: 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:34: note: in expansion of macro ‘RTE_FLOW_MASK_ARRAY_SIZE’
  452 |         uint64_t flow_types_mask[RTE_FLOW_MASK_ARRAY_SIZE];