[RFC] eal: make rte_rand() MT safe
Checks
Commit Message
The rte_rand() documentation left it unspecified if the rte_rand() was
multi-thread safe or not, and the implementation (based on lrand48())
was not.
This commit makes rte_rand() safe to use from any lcore thread by
using lrand48_r() and per-lcore random state structs. Besides the
obvious improvement in terms of correctness (for concurrent users),
this also much improves rte_rand() performance, since the threads no
longer shares state. For the single-threaded case, this patch causes
~10% rte_rand() performance degradation.
rte_srand() is left multi-thread unsafe, and external synchronization
is required to serialize rte_sand() calls from different lcore
threads, and a rte_srand() call with rte_rand() calls made by other
lcore threads.
The assumption is that the random number generators will be seeded
once, during startup.
Bugzilla ID: 114
Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
lib/librte_eal/common/include/rte_random.h | 25 ++++-----
lib/librte_eal/common/meson.build | 1 +
lib/librte_eal/common/rte_random.c | 65 ++++++++++++++++++++++
lib/librte_eal/freebsd/eal/Makefile | 1 +
lib/librte_eal/freebsd/eal/eal.c | 2 -
lib/librte_eal/linux/eal/Makefile | 1 +
lib/librte_eal/linux/eal/eal.c | 2 -
lib/librte_eal/rte_eal_version.map | 2 +
8 files changed, 80 insertions(+), 19 deletions(-)
create mode 100644 lib/librte_eal/common/rte_random.c
Comments
On 2019-04-05 15:45, Mattias Rönnblom wrote:
> The rte_rand() documentation left it unspecified if the rte_rand() was
> multi-thread safe or not, and the implementation (based on lrand48())
> was not.
>
> This commit makes rte_rand() safe to use from any lcore thread by
> using lrand48_r() and per-lcore random state structs. Besides the
> obvious improvement in terms of correctness (for concurrent users),
> this also much improves rte_rand() performance, since the threads no
> longer shares state. For the single-threaded case, this patch causes
> ~10% rte_rand() performance degradation.
>
It's a little unclear to me, if lrand48_r() exists in FreeBSD or not.
Could someone confirm?
Another question I have is in which section of the version.map file the
new symbols should go. Experimental, or 19.05?
The source interface is backward compatible, but the functions are no
longer inline functions in the header file, and thus needs to go
somewhere to be properly exported.
On Fri, Apr 05, 2019 at 03:51:39PM +0200, Mattias Rönnblom wrote:
> On 2019-04-05 15:45, Mattias Rönnblom wrote:
> > The rte_rand() documentation left it unspecified if the rte_rand() was
> > multi-thread safe or not, and the implementation (based on lrand48())
> > was not.
> >
> > This commit makes rte_rand() safe to use from any lcore thread by
> > using lrand48_r() and per-lcore random state structs. Besides the
> > obvious improvement in terms of correctness (for concurrent users),
> > this also much improves rte_rand() performance, since the threads no
> > longer shares state. For the single-threaded case, this patch causes
> > ~10% rte_rand() performance degradation.
> >
>
> It's a little unclear to me, if lrand48_r() exists in FreeBSD or not. Could
> someone confirm?
>
Nothing shows up for me in the man pages for such a function on FreeBSD 12,
so I suspect they aren't available.
> Another question I have is in which section of the version.map file the new
> symbols should go. Experimental, or 19.05?
>
I think it should be 19.05. Since the APIs have been around as inline
functions for some time now, I don't see the point of having them be
experimental for a time.
> The source interface is backward compatible, but the functions are no longer
> inline functions in the header file, and thus needs to go somewhere to be
> properly exported.
Regards,
/Bruce
On 2019-04-05 16:28, Bruce Richardson wrote:
> On Fri, Apr 05, 2019 at 03:51:39PM +0200, Mattias Rönnblom wrote:
>> On 2019-04-05 15:45, Mattias Rönnblom wrote:
>>> The rte_rand() documentation left it unspecified if the rte_rand() was
>>> multi-thread safe or not, and the implementation (based on lrand48())
>>> was not.
>>>
>>> This commit makes rte_rand() safe to use from any lcore thread by
>>> using lrand48_r() and per-lcore random state structs. Besides the
>>> obvious improvement in terms of correctness (for concurrent users),
>>> this also much improves rte_rand() performance, since the threads no
>>> longer shares state. For the single-threaded case, this patch causes
>>> ~10% rte_rand() performance degradation.
>>>
>>
>> It's a little unclear to me, if lrand48_r() exists in FreeBSD or not. Could
>> someone confirm?
>>
> Nothing shows up for me in the man pages for such a function on FreeBSD 12,
> so I suspect they aren't available.
>
Could arc4random(3) be a good replacement on FreeBSD? It "can be called
in almost all coding environments, including pthreads(3)" according to
the man page, so assume it's MT safe.
On Fri, 5 Apr 2019 15:45:42 +0200
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> The rte_rand() documentation left it unspecified if the rte_rand() was
> multi-thread safe or not, and the implementation (based on lrand48())
> was not.
>
> This commit makes rte_rand() safe to use from any lcore thread by
> using lrand48_r() and per-lcore random state structs. Besides the
> obvious improvement in terms of correctness (for concurrent users),
> this also much improves rte_rand() performance, since the threads no
> longer shares state. For the single-threaded case, this patch causes
> ~10% rte_rand() performance degradation.
>
> rte_srand() is left multi-thread unsafe, and external synchronization
> is required to serialize rte_sand() calls from different lcore
> threads, and a rte_srand() call with rte_rand() calls made by other
> lcore threads.
>
> The assumption is that the random number generators will be seeded
> once, during startup.
>
> Bugzilla ID: 114
>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
> lib/librte_eal/common/include/rte_random.h | 25 ++++-----
> lib/librte_eal/common/meson.build | 1 +
> lib/librte_eal/common/rte_random.c | 65 ++++++++++++++++++++++
> lib/librte_eal/freebsd/eal/Makefile | 1 +
> lib/librte_eal/freebsd/eal/eal.c | 2 -
> lib/librte_eal/linux/eal/Makefile | 1 +
> lib/librte_eal/linux/eal/eal.c | 2 -
> lib/librte_eal/rte_eal_version.map | 2 +
> 8 files changed, 80 insertions(+), 19 deletions(-)
> create mode 100644 lib/librte_eal/common/rte_random.c
>
> diff --git a/lib/librte_eal/common/include/rte_random.h b/lib/librte_eal/common/include/rte_random.h
> index b2ca1c209..bca85a672 100644
> --- a/lib/librte_eal/common/include/rte_random.h
> +++ b/lib/librte_eal/common/include/rte_random.h
> @@ -16,7 +16,6 @@ extern "C" {
> #endif
>
> #include <stdint.h>
> -#include <stdlib.h>
>
> /**
> * Seed the pseudo-random generator.
> @@ -25,14 +24,15 @@ extern "C" {
> * value. It may need to be re-seeded by the user with a real random
> * value.
> *
> + * This function is not multi-thread safe in regards to other
> + * rte_srand() calls, nor is it in relation to concurrent rte_rand()
> + * calls.
> + *
> * @param seedval
> * The value of the seed.
> */
> -static inline void
> -rte_srand(uint64_t seedval)
> -{
> - srand48((long)seedval);
> -}
> +void
> +rte_srand(uint64_t seedval);
>
> /**
> * Get a pseudo-random value.
> @@ -41,18 +41,13 @@ rte_srand(uint64_t seedval)
> * congruential algorithm and 48-bit integer arithmetic, called twice
> * to generate a 64-bit value.
> *
> + * If called from lcore threads, this function is thread-safe.
> + *
> * @return
> * A pseudo-random value between 0 and (1<<64)-1.
> */
> -static inline uint64_t
> -rte_rand(void)
> -{
> - uint64_t val;
> - val = (uint64_t)lrand48();
> - val <<= 32;
> - val += (uint64_t)lrand48();
> - return val;
> -}
> +uint64_t
> +rte_rand(void);
>
> #ifdef __cplusplus
> }
> diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
> index 0670e4102..bafd23207 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -35,6 +35,7 @@ common_sources = files(
> 'rte_keepalive.c',
> 'rte_malloc.c',
> 'rte_option.c',
> + 'rte_random.c',
> 'rte_reciprocal.c',
> 'rte_service.c'
> )
> diff --git a/lib/librte_eal/common/rte_random.c b/lib/librte_eal/common/rte_random.c
> new file mode 100644
> index 000000000..9d519d03b
> --- /dev/null
> +++ b/lib/librte_eal/common/rte_random.c
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation
> + * Copyright(c) 2019 Ericsson AB
> + */
> +
> +#include <stdlib.h>
> +
> +#include <rte_cycles.h>
> +#include <rte_eal.h>
> +#include <rte_lcore.h>
> +#include <rte_random.h>
> +
> +struct rte_rand_data
> +{
> + struct drand48_data data;
> +} __rte_cache_aligned;
> +
> +static struct rte_rand_data rand_data[RTE_MAX_LCORE];
> +
> +void
> +rte_srand(uint64_t seedval)
> +{
> + unsigned i;
> +
> + /* give the different lcores a different seed, to avoid a
> + situation where they generate the same sequence */
> + for (i = 0; i < RTE_MAX_LCORE; i++)
> + srand48_r((long)seedval + i, &rand_data[i].data);
> +}
> +
> +static inline uint32_t
> +__rte_rand48(struct drand48_data *data)
> +{
> + long res;
> +
> + lrand48_r(data, &res);
> +
> + return (uint32_t)res;
> +}
> +
> +uint64_t
> +rte_rand(void)
> +{
> + unsigned lcore_id;
> + struct drand48_data *data;
> + uint64_t val;
> +
> + lcore_id = rte_lcore_id();
> +
> + if (unlikely(lcore_id == LCORE_ID_ANY))
> + lcore_id = rte_get_master_lcore();
> +
> + data = &rand_data[lcore_id].data;
> +
> + val = __rte_rand48(data);
> + val <<= 32;
> + val += __rte_rand48(data);
> +
> + return val;
> +}
> +
> +RTE_INIT(rte_rand_init)
> +{
> + rte_srand(rte_get_timer_cycles());
> +}
rand48 is a terrible PRNG, why not use something better?
Similar discussion in Linux kernel pointed at:
http://www.pcg-random.org/posts/some-prng-implementations.html
Mail thread here:
https://www.spinics.net/lists/netdev/msg560231.html
On 2019-04-05 18:57, Stephen Hemminger wrote:
>
> rand48 is a terrible PRNG, why not use something better?
>
> Similar discussion in Linux kernel pointed at:
> http://www.pcg-random.org/posts/some-prng-implementations.html
>
> Mail thread here:
> https://www.spinics.net/lists/netdev/msg560231.html
>
DPDK was already using lrand48(), I primarily wanted to address
lrand48()'s lack of MT safety, nothing else.
That said, maybe the easiest way to maintain the current API, provide MT
safety and have something portable is for DPDK to carry its own random
number generator, instead of relying on libc.
Maybe an ARC4 port from BSD? But instead of pulling entropy from the
kernel, let the user give the seed, like the current DPDK APIs permit.
You could deprecate rte_srand(), but I would vote against such a move,
because its sometimes useful to able to have something that is "random",
yet reproducible.
On Fri, 5 Apr 2019 20:04:28 +0200
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> On 2019-04-05 18:57, Stephen Hemminger wrote:
> >
> > rand48 is a terrible PRNG, why not use something better?
> >
> > Similar discussion in Linux kernel pointed at:
> > http://www.pcg-random.org/posts/some-prng-implementations.html
> >
> > Mail thread here:
> > https://www.spinics.net/lists/netdev/msg560231.html
> >
>
> DPDK was already using lrand48(), I primarily wanted to address
> lrand48()'s lack of MT safety, nothing else.
>
> That said, maybe the easiest way to maintain the current API, provide MT
> safety and have something portable is for DPDK to carry its own random
> number generator, instead of relying on libc.
>
> Maybe an ARC4 port from BSD? But instead of pulling entropy from the
> kernel, let the user give the seed, like the current DPDK APIs permit.
>
> You could deprecate rte_srand(), but I would vote against such a move,
> because its sometimes useful to able to have something that is "random",
> yet reproducible.
Read the discussion link about ARC4. http://www.pcg-random.org/
As a general-purpose PRNG, it is rather slow, and it is also slow by
the standards of modern cryptographic PRNGs and is also considered too
weak to use for cryptographic purposes. It is, however, of historical
interest and can be useful in testing to see how sensitive a algorithms
are to PRNG speed.
Using ARC4 replaces a one legacy one with another.
On 2019-04-05 22:50, Stephen Hemminger wrote:
>
> Read the discussion link about ARC4. http://www.pcg-random.org/
>
> As a general-purpose PRNG, it is rather slow, and it is also slow by
> the standards of modern cryptographic PRNGs and is also considered too
> weak to use for cryptographic purposes. It is, however, of historical
> interest and can be useful in testing to see how sensitive a algorithms
> are to PRNG speed.
>
> Using ARC4 replaces a one legacy one with another.
>
Yes, I agree.
After looking at the code, I learned that - seemingly - most
arc4random() implementations aren't using ARC4, but ChaCha.
Would it be unfortunate from a export control point of view to include
cipher-based random generators in DPDK?
@@ -16,7 +16,6 @@ extern "C" {
#endif
#include <stdint.h>
-#include <stdlib.h>
/**
* Seed the pseudo-random generator.
@@ -25,14 +24,15 @@ extern "C" {
* value. It may need to be re-seeded by the user with a real random
* value.
*
+ * This function is not multi-thread safe in regards to other
+ * rte_srand() calls, nor is it in relation to concurrent rte_rand()
+ * calls.
+ *
* @param seedval
* The value of the seed.
*/
-static inline void
-rte_srand(uint64_t seedval)
-{
- srand48((long)seedval);
-}
+void
+rte_srand(uint64_t seedval);
/**
* Get a pseudo-random value.
@@ -41,18 +41,13 @@ rte_srand(uint64_t seedval)
* congruential algorithm and 48-bit integer arithmetic, called twice
* to generate a 64-bit value.
*
+ * If called from lcore threads, this function is thread-safe.
+ *
* @return
* A pseudo-random value between 0 and (1<<64)-1.
*/
-static inline uint64_t
-rte_rand(void)
-{
- uint64_t val;
- val = (uint64_t)lrand48();
- val <<= 32;
- val += (uint64_t)lrand48();
- return val;
-}
+uint64_t
+rte_rand(void);
#ifdef __cplusplus
}
@@ -35,6 +35,7 @@ common_sources = files(
'rte_keepalive.c',
'rte_malloc.c',
'rte_option.c',
+ 'rte_random.c',
'rte_reciprocal.c',
'rte_service.c'
)
new file mode 100644
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2014 Intel Corporation
+ * Copyright(c) 2019 Ericsson AB
+ */
+
+#include <stdlib.h>
+
+#include <rte_cycles.h>
+#include <rte_eal.h>
+#include <rte_lcore.h>
+#include <rte_random.h>
+
+struct rte_rand_data
+{
+ struct drand48_data data;
+} __rte_cache_aligned;
+
+static struct rte_rand_data rand_data[RTE_MAX_LCORE];
+
+void
+rte_srand(uint64_t seedval)
+{
+ unsigned i;
+
+ /* give the different lcores a different seed, to avoid a
+ situation where they generate the same sequence */
+ for (i = 0; i < RTE_MAX_LCORE; i++)
+ srand48_r((long)seedval + i, &rand_data[i].data);
+}
+
+static inline uint32_t
+__rte_rand48(struct drand48_data *data)
+{
+ long res;
+
+ lrand48_r(data, &res);
+
+ return (uint32_t)res;
+}
+
+uint64_t
+rte_rand(void)
+{
+ unsigned lcore_id;
+ struct drand48_data *data;
+ uint64_t val;
+
+ lcore_id = rte_lcore_id();
+
+ if (unlikely(lcore_id == LCORE_ID_ANY))
+ lcore_id = rte_get_master_lcore();
+
+ data = &rand_data[lcore_id].data;
+
+ val = __rte_rand48(data);
+ val <<= 32;
+ val += __rte_rand48(data);
+
+ return val;
+}
+
+RTE_INIT(rte_rand_init)
+{
+ rte_srand(rte_get_timer_cycles());
+}
@@ -69,6 +69,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += malloc_mp.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_keepalive.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_option.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_service.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_random.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_reciprocal.c
# from arch dir
@@ -727,8 +727,6 @@ rte_eal_init(int argc, char **argv)
#endif
}
- rte_srand(rte_rdtsc());
-
/* in secondary processes, memory init may allocate additional fbarrays
* not present in primary processes, so to avoid any potential issues,
* initialize memzones first.
@@ -77,6 +77,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += malloc_mp.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_keepalive.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_option.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_service.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_random.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_reciprocal.c
# from arch dir
@@ -1083,8 +1083,6 @@ rte_eal_init(int argc, char **argv)
#endif
}
- rte_srand(rte_rdtsc());
-
if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0) {
rte_eal_init_alert("Cannot init logging.");
rte_errno = ENOMEM;
@@ -366,10 +366,12 @@ EXPERIMENTAL {
rte_mp_request_async;
rte_mp_sendmsg;
rte_option_register;
+ rte_rand;
rte_realloc_socket;
rte_service_lcore_attr_get;
rte_service_lcore_attr_reset_all;
rte_service_may_be_active;
rte_socket_count;
rte_socket_id_by_idx;
+ rte_srand;
};