Re: [dpdk-dev] [RFC] eal: make rte_rand() MT safe

[Stephen Hemminger <stephen@networkplumber.org>]

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