DPDK patches and discussions
 help / color / mirror / Atom feed
* RE: [PATCH v2] eal: avoid issues in macro expansion of alignment
@ 2023-07-14 21:11 Morten Brørup
  0 siblings, 0 replies; 6+ messages in thread
From: Morten Brørup @ 2023-07-14 21:11 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, pbhagavatula

[-- Attachment #1: Type: text/plain, Size: 4728 bytes --]

Stephen, perhaps you can solve it using __builtin_constant_p() in the macro.-MortenSent from smartphone. Please excuse brevity and spelling.
-------- Oprindelig besked --------Fra: Stephen Hemminger <stephen@networkplumber.org> Dato: 14.07.2023  18.37  (GMT+01:00) Til: Morten Brørup <mb@smartsharesystems.com> Cc: dev@dpdk.org, pbhagavatula@marvell.com Emne: Re: [PATCH v2] eal: avoid issues in macro expansion of alignment On Wed, 5 Jul 2023 00:16:50 +0200Morten 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];

[-- Attachment #2: Type: text/html, Size: 7561 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] eal: avoid issues in macro expansion of alignment
@ 2023-07-03 18:55 Stephen Hemminger
  2023-07-03 23:23 ` [PATCH v2] " Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2023-07-03 18:55 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, pbhagavatula, Keith Wiles

RTE_ALIGN_MUL_NEAR is a macro so the cycle arguement 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>
---
 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-07-14 21:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-14 21:11 [PATCH v2] eal: avoid issues in macro expansion of alignment Morten Brørup
  -- strict thread matches above, loose matches on Subject: below --
2023-07-03 18:55 [PATCH] " Stephen Hemminger
2023-07-03 23:23 ` [PATCH v2] " Stephen Hemminger
2023-07-04  8:43   ` Morten Brørup
2023-07-04 16:00     ` Stephen Hemminger
2023-07-04 22:16       ` Morten Brørup
2023-07-14 16:37         ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).