DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros
@ 2021-05-09 17:18 pbhagavatula
  2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
  2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
  0 siblings, 2 replies; 20+ messages in thread
From: pbhagavatula @ 2021-05-09 17:18 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, Pavan Nikhilesh, stable, Tyler Retzlaff

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Avoid expanding parameters inside the macro multiple times.
For example:
	RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
Here rte_rdtsc() call is expanded multiple times in the macro
causing it to return different values that leads to unintended
side effects.

Also, update common_autotest to detect macro side effects.

Fixes: f56e551485d5 ("eal: add macro to align value to the nearest multiple")
Cc: stable@dpdk.org

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 Retainded original author signoff, split patches as some of the changes
 involved in modifying drivers.

 app/test/test_common.c       | 27 ++++++++++++++++++++++++++-
 lib/eal/include/rte_common.h | 28 +++++++++++++++++++---------
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 12bd1cad90..0dbb87e741 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)
@@ -29,7 +36,19 @@ test_macros(int __rte_unused unused_parm)
 	{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 +66,13 @@ 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);
+	TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_CEIL, unsigned int, unsigned int);
+	TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_FLOOR, unsigned int, unsigned int);
+	TEST_SIDE_EFFECT_2(RTE_ALIGN_MUL_NEAR, unsigned int, unsigned int);
+	return ret;
 }

 static int
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5..a142596587 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -329,27 +329,37 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
  * value will be of the same type as the first parameter and will be no lower
  * than the first parameter.
  */
-#define RTE_ALIGN_MUL_CEIL(v, mul) \
-	((((v) + (typeof(v))(mul) - 1) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_CEIL(v, mul)                                             \
+	__extension__({                                                        \
+		typeof(v) _vc = (v);                                           \
+		typeof(v) _mc = (mul);                                         \
+		((_vc + _mc - 1) / _mc) * _mc;                                 \
+	})

 /**
  * Macro to align a value to the multiple of given value. The resultant
  * value will be of the same type as the first parameter and will be no higher
  * than the first parameter.
  */
-#define RTE_ALIGN_MUL_FLOOR(v, mul) \
-	(((v) / ((typeof(v))(mul))) * (typeof(v))(mul))
+#define RTE_ALIGN_MUL_FLOOR(v, mul)                                            \
+	__extension__({                                                        \
+		typeof(v) _vf = (v);                                           \
+		typeof(v) _mf = (mul);                                         \
+		(_vf / _mf) * _mf;                                             \
+	})

 /**
  * Macro to align value to the nearest multiple of the given value.
  * The resultant value might be greater than or less than the first parameter
  * whichever difference is the lowest.
  */
-#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;	\
+#define RTE_ALIGN_MUL_NEAR(v, mul)                                             \
+	__extension__({                                                        \
+		typeof(v) _v = (v);                                            \
+		typeof(v) _m = (mul);                                          \
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(_v, _m);                 \
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(_v, _m);                   \
+		(ceil - _v) > (_v - floor) ? floor : ceil;                     \
 	})

 /**
--
2.17.1


^ permalink raw reply	[flat|nested] 20+ messages in thread
* RE: [PATCH v2] eal: avoid issues in macro expansion of alignment
@ 2023-07-14 21:11 Morten Brørup
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread
end of thread, other threads:[~2023-07-14 21:11 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-09 17:18 [dpdk-dev] [PATCH 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-09 17:18 ` [dpdk-dev] [PATCH 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-09 19:39   ` Stephen Hemminger
2021-05-10  9:50     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2021-05-10 13:34       ` David Marchand
2021-05-10 14:02 ` [dpdk-dev] [PATCH v2 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 14:02   ` [dpdk-dev] [PATCH v2 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-10 19:40   ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros pbhagavatula
2021-05-10 19:40     ` [dpdk-dev] [PATCH v3 2/2] eal: fix side effects in ptr align macros pbhagavatula
2021-05-11  2:12       ` Wang, Haiyue
2021-05-11  8:44         ` Pavan Nikhilesh Bhagavatula
2021-05-12  1:13           ` Wang, Haiyue
2023-07-03 18:55     ` [PATCH] eal: avoid issues in macro expansion of alignment 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
2023-07-04 16:01     ` [dpdk-dev] [PATCH v3 1/2] eal: fix side effects in align mul macros Stephen Hemminger
2023-07-14 21:11 [PATCH v2] eal: avoid issues in macro expansion of alignment Morten Brørup

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).