On Wed, 5 Jul 2023 00:16:50 +0200 Morten Brørup <m...@smartsharesystems.com> wrote:
> > 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 > 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];
