> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Tuesday, 4 July 2023 18.01
>
> On Tue, 4 Jul 2023 10:43:40 +0200
> Morten Brørup <m...@smartsharesystems.com> wrote:
>
> > > From: Stephen Hemminger [mailto:step...@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: pbhagavat...@marvell.com
> > > Signed-off-by: Stephen Hemminger <step...@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
