[PATCH] random: initialize the random state for non-eal lcores

[PATCH] random: initialize the random state for non-eal lcores

[Stephen Hemminger]

The per-lcore PRNG was not initializing the rand_state of all
the lcores. Any usage of rte_random by a non-EAL lcore would
use rand_states[RTE_MAX_LCORE] which was never initialized.

Fix by using RTE_DIM() which will get all lcores.

Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
Cc: mattias.ronnblom@ericsson.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/mlx5/linux/mlx5_common_os.c | 2 +-
 lib/eal/common/rte_random.c                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/common/mlx5/linux/mlx5_common_os.c b/drivers/common/mlx5/linux/mlx5_common_os.c
index 2ebb8ac8b6e5..7260c1a19fd3 100644
--- a/drivers/common/mlx5/linux/mlx5_common_os.c
+++ b/drivers/common/mlx5/linux/mlx5_common_os.c
@@ -266,7 +266,7 @@ mlx5_glue_path(char *buf, size_t size)
 		goto error;
 	return buf;
 error:
-	RTE_LOG(ERR, PMD, "unable to append \"-glue\" to last component of"
+	DRV_LOG(ERR, "unable to append \"-glue\" to last component of"
 		" RTE_EAL_PMD_PATH (\"" RTE_EAL_PMD_PATH "\"), please"
 		" re-configure DPDK");
 	return NULL;
diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 53636331a27b..812e5b4757b5 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
 	unsigned int lcore_id;
 
 	/* add lcore_id to seed to avoid having the same sequence */
-	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
+	for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
 		__rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
 }
 
-- 
2.39.2

RE: [PATCH] random: initialize the random state for non-eal lcores

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 6 September 2023 17.53
> 
> The per-lcore PRNG was not initializing the rand_state of all
> the lcores. Any usage of rte_random by a non-EAL lcore would
> use rand_states[RTE_MAX_LCORE] which was never initialized.
> 
> Fix by using RTE_DIM() which will get all lcores.
> 
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> Cc: mattias.ronnblom@ericsson.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/mlx5/linux/mlx5_common_os.c | 2 +-

With the unrelated MLX5 stuff removed,

Acked-by: Morten Brørup <mb@smartsharesystems.com>

>  lib/eal/common/rte_random.c                | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH] random: initialize the random state for non-eal lcores

[Stephen Hemminger]

On Wed,  6 Sep 2023 08:53:02 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> diff --git a/drivers/common/mlx5/linux/mlx5_common_os.c b/drivers/common/mlx5/linux/mlx5_common_os.c
> index 2ebb8ac8b6e5..7260c1a19fd3 100644
> --- a/drivers/common/mlx5/linux/mlx5_common_os.c
> +++ b/drivers/common/mlx5/linux/mlx5_common_os.c
> @@ -266,7 +266,7 @@ mlx5_glue_path(char *buf, size_t size)
>  		goto error;
>  	return buf;
>  error:
> -	RTE_LOG(ERR, PMD, "unable to append \"-glue\" to last component of"
> +	DRV_LOG(ERR, "unable to append \"-glue\" to last component of"
>  		" RTE_EAL_PMD_PATH (\"" RTE_EAL_PMD_PATH "\"), please"
>  		" re-configure DPDK");
>  	return NULL;

Not sure how that got in, ignore it.

[PATCH v2 0/2] fixes to rte_random for non-EAL threads

[Stephen Hemminger]

While examining the code for rte_random, noticed a couple of
existing bugs around use of rte_rand() by non-EAL threads

Stephen Hemminger (2):
  random: initialize the random state for non-EAL threads
  random: make rte_rand() thread safe for non-EAL threads

 lib/eal/common/rte_random.c | 56 ++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 19 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] random: initialize the random state for non-EAL threads

[Stephen Hemminger]

The per-lcore PRNG was not initializing the rand_state of all
the lcores. Any usage of rte_random by a non-EAL lcore would
use rand_states[RTE_MAX_LCORE] which was never initialized.

Fix by using RTE_DIM() which will get all lcores.

Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
Cc: mattias.ronnblom@ericsson.com
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/common/rte_random.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 53636331a27b..812e5b4757b5 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
 	unsigned int lcore_id;
 
 	/* add lcore_id to seed to avoid having the same sequence */
-	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
+	for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
 		__rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
 }
 
-- 
2.39.2

RE: [PATCH v2 1/2] random: initialize the random state for non-EAL threads

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 7 September 2023 17.25
> 
> The per-lcore PRNG was not initializing the rand_state of all
> the lcores. Any usage of rte_random by a non-EAL lcore would
> use rand_states[RTE_MAX_LCORE] which was never initialized.
> 
> Fix by using RTE_DIM() which will get all lcores.
> 
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> Cc: mattias.ronnblom@ericsson.com
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/eal/common/rte_random.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> index 53636331a27b..812e5b4757b5 100644
> --- a/lib/eal/common/rte_random.c
> +++ b/lib/eal/common/rte_random.c
> @@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
>  	unsigned int lcore_id;
> 
>  	/* add lcore_id to seed to avoid having the same sequence */
> -	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> +	for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
>  		__rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
>  }
> 
> --
> 2.39.2

Now also...

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 1/2] random: initialize the random state for non-EAL threads

[Mattias Rönnblom]

On 2023-09-07 17:24, Stephen Hemminger wrote:
> The per-lcore PRNG was not initializing the rand_state of all
> the lcores. Any usage of rte_random by a non-EAL lcore would

"/../ by an unregistered non-EAL thread /../"

> use rand_states[RTE_MAX_LCORE] which was never initialized.
> 
> Fix by using RTE_DIM() which will get all lcores.
> 
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> Cc: mattias.ronnblom@ericsson.com
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/eal/common/rte_random.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> index 53636331a27b..812e5b4757b5 100644
> --- a/lib/eal/common/rte_random.c
> +++ b/lib/eal/common/rte_random.c
> @@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
>   	unsigned int lcore_id;
>   
>   	/* add lcore_id to seed to avoid having the same sequence */
> -	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> +	for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
>   		__rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
>   }
>   

With the above-mentioned commit message rewording:

Acked-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

Re: [PATCH v2 1/2] random: initialize the random state for non-EAL threads

[Stephen Hemminger]

On Mon, 2 Oct 2023 14:27:48 +0200
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> On 2023-09-07 17:24, Stephen Hemminger wrote:
> > The per-lcore PRNG was not initializing the rand_state of all
> > the lcores. Any usage of rte_random by a non-EAL lcore would  
> 
> "/../ by an unregistered non-EAL thread /../"
> 
> > use rand_states[RTE_MAX_LCORE] which was never initialized.

Sure, that is a more precise wording.

Re: [PATCH v2 1/2] random: initialize the random state for non-EAL threads

[David Marchand]

On Mon, Oct 2, 2023 at 2:28 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2023-09-07 17:24, Stephen Hemminger wrote:
> > The per-lcore PRNG was not initializing the rand_state of all
> > the lcores. Any usage of rte_random by a non-EAL lcore would
>
> "/../ by an unregistered non-EAL thread /../"
>
> > use rand_states[RTE_MAX_LCORE] which was never initialized.
> >
> > Fix by using RTE_DIM() which will get all lcores.
> >
> > Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> > Cc: mattias.ronnblom@ericsson.com
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >   lib/eal/common/rte_random.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> > index 53636331a27b..812e5b4757b5 100644
> > --- a/lib/eal/common/rte_random.c
> > +++ b/lib/eal/common/rte_random.c
> > @@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
> >       unsigned int lcore_id;
> >
> >       /* add lcore_id to seed to avoid having the same sequence */
> > -     for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> > +     for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
> >               __rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
> >   }
> >
>
> With the above-mentioned commit message rewording:
>
> Acked-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>

Applied this patch.
The second patch is marked as rejected in pw, in favor of a followup doc patch.

Thanks.

-- 
David Marchand

[PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Stephen Hemminger]

Add missing locking so that if two non-EAL threads call rte_rand()
they will not corrupt the per-thread state.

Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/common/rte_random.c | 54 ++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 812e5b4757b5..02b6b6b97bc0 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -11,6 +11,7 @@
 #include <rte_branch_prediction.h>
 #include <rte_cycles.h>
 #include <rte_lcore.h>
+#include <rte_spinlock.h>
 #include <rte_random.h>
 
 struct rte_rand_state {
@@ -21,6 +22,9 @@ struct rte_rand_state {
 	uint64_t z5;
 } __rte_cache_aligned;
 
+/* Used for thread safety for non EAL threads. */
+static rte_spinlock_t rte_rand_lock = RTE_SPINLOCK_INITIALIZER;
+
 /* One instance each for every lcore id-equipped thread, and one
  * additional instance to be shared by all others threads (i.e., all
  * unregistered non-EAL threads).
@@ -124,20 +128,32 @@ struct rte_rand_state *__rte_rand_get_state(void)
 	idx = rte_lcore_id();
 
 	/* last instance reserved for unregistered non-EAL threads */
-	if (unlikely(idx == LCORE_ID_ANY))
+	if (unlikely(idx == LCORE_ID_ANY)) {
 		idx = RTE_MAX_LCORE;
+		rte_spinlock_lock(&rte_rand_lock);
+	}
 
 	return &rand_states[idx];
 }
 
+static __rte_always_inline
+void __rte_rand_put_state(struct rte_rand_state *state)
+{
+	if (state == &rand_states[RTE_MAX_LCORE])
+		rte_spinlock_unlock(&rte_rand_lock);
+}
+
 uint64_t
 rte_rand(void)
 {
 	struct rte_rand_state *state;
+	uint64_t res;
 
 	state = __rte_rand_get_state();
+	res = __rte_rand_lfsr258(state);
+	__rte_rand_put_state(state);
 
-	return __rte_rand_lfsr258(state);
+	return res;
 }
 
 uint64_t
@@ -159,22 +175,24 @@ rte_rand_max(uint64_t upper_bound)
 	/* Handle power-of-2 upper_bound as a special case, since it
 	 * has no bias issues.
 	 */
-	if (unlikely(ones == 1))
-		return __rte_rand_lfsr258(state) & (upper_bound - 1);
-
-	/* The approach to avoiding bias is to create a mask that
-	 * stretches beyond the request value range, and up to the
-	 * next power-of-2. In case the masked generated random value
-	 * is equal to or greater than the upper bound, just discard
-	 * the value and generate a new one.
-	 */
-
-	leading_zeros = rte_clz64(upper_bound);
-	mask >>= leading_zeros;
-
-	do {
-		res = __rte_rand_lfsr258(state) & mask;
-	} while (unlikely(res >= upper_bound));
+	if (unlikely(ones == 1)) {
+		res = __rte_rand_lfsr258(state) & (upper_bound - 1);
+	} else {
+		/* The approach to avoiding bias is to create a mask that
+		 * stretches beyond the request value range, and up to the
+		 * next power-of-2. In case the masked generated random value
+		 * is equal to or greater than the upper bound, just discard
+		 * the value and generate a new one.
+		 */
+
+		leading_zeros = rte_clz64(upper_bound);
+		mask >>= leading_zeros;
+
+		do {
+			res = __rte_rand_lfsr258(state) & mask;
+		} while (unlikely(res >= upper_bound));
+	}
+	__rte_rand_put_state(state);
 
 	return res;
 }
-- 
2.39.2

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Stephen Hemminger]

On Thu,  7 Sep 2023 08:24:56 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

>  
> +static __rte_always_inline
> +void __rte_rand_put_state(struct rte_rand_state *state)
> +{
> +	if (state == &rand_states[RTE_MAX_LCORE])
> +		rte_spinlock_unlock(&rte_rand_lock);
> +}

Conditional locking like this make clang lock analyzer unhappy though.

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[David Marchand]

On Thu, Sep 7, 2023 at 5:48 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu,  7 Sep 2023 08:24:56 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> >
> > +static __rte_always_inline
> > +void __rte_rand_put_state(struct rte_rand_state *state)
> > +{
> > +     if (state == &rand_states[RTE_MAX_LCORE])
> > +             rte_spinlock_unlock(&rte_rand_lock);
> > +}
>
> Conditional locking like this make clang lock analyzer unhappy though.

Ugly, but some macro can do the job...

diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 02b6b6b97b..3f2a4830fd 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -128,20 +128,22 @@ struct rte_rand_state *__rte_rand_get_state(void)
        idx = rte_lcore_id();

        /* last instance reserved for unregistered non-EAL threads */
-       if (unlikely(idx == LCORE_ID_ANY)) {
+       if (unlikely(idx == LCORE_ID_ANY))
                idx = RTE_MAX_LCORE;
-               rte_spinlock_lock(&rte_rand_lock);
-       }

        return &rand_states[idx];
 }

-static __rte_always_inline
-void __rte_rand_put_state(struct rte_rand_state *state)
-{
-       if (state == &rand_states[RTE_MAX_LCORE])
-               rte_spinlock_unlock(&rte_rand_lock);
-}
+#define PROTECT_NON_EAL_THREADS(...) do { \
+       unsigned int idx = rte_lcore_id(); \
+       if (idx == LCORE_ID_ANY) { \
+               rte_spinlock_lock(&rte_rand_lock); \
+               __VA_ARGS__ \
+               rte_spinlock_unlock(&rte_rand_lock); \
+       } else { \
+               __VA_ARGS__ \
+       } \
+} while (0)

 uint64_t
 rte_rand(void)
@@ -149,9 +151,10 @@ rte_rand(void)
        struct rte_rand_state *state;
        uint64_t res;

+       PROTECT_NON_EAL_THREADS(
        state = __rte_rand_get_state();
        res = __rte_rand_lfsr258(state);
-       __rte_rand_put_state(state);
+       );

        return res;
 }
@@ -168,6 +171,7 @@ rte_rand_max(uint64_t upper_bound)
        if (unlikely(upper_bound < 2))
                return 0;

+       PROTECT_NON_EAL_THREADS(
        state = __rte_rand_get_state();

        ones = rte_popcount64(upper_bound);
@@ -192,7 +196,7 @@ rte_rand_max(uint64_t upper_bound)
                        res = __rte_rand_lfsr258(state) & mask;
                } while (unlikely(res >= upper_bound));
        }
-       __rte_rand_put_state(state);
+       );

        return res;
 }


-- 
David Marchand

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Mattias Rönnblom]

On 2023-09-07 17:24, Stephen Hemminger wrote:
> Add missing locking so that if two non-EAL threads call rte_rand()
> they will not corrupt the per-thread state.
> 
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")

The API documentation clearly states that no MT safety guarantees are 
given for unregistered non-EAL threads. So this patch doesn't fix anything.

rte_rand() is MT safe for *registered* non-EAL threads.

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/eal/common/rte_random.c | 54 ++++++++++++++++++++++++-------------
>   1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> index 812e5b4757b5..02b6b6b97bc0 100644
> --- a/lib/eal/common/rte_random.c
> +++ b/lib/eal/common/rte_random.c
> @@ -11,6 +11,7 @@
>   #include <rte_branch_prediction.h>
>   #include <rte_cycles.h>
>   #include <rte_lcore.h>
> +#include <rte_spinlock.h>
>   #include <rte_random.h>
>   
>   struct rte_rand_state {
> @@ -21,6 +22,9 @@ struct rte_rand_state {
>   	uint64_t z5;
>   } __rte_cache_aligned;
>   
> +/* Used for thread safety for non EAL threads. */
> +static rte_spinlock_t rte_rand_lock = RTE_SPINLOCK_INITIALIZER;
> +
>   /* One instance each for every lcore id-equipped thread, and one
>    * additional instance to be shared by all others threads (i.e., all
>    * unregistered non-EAL threads).
> @@ -124,20 +128,32 @@ struct rte_rand_state *__rte_rand_get_state(void)
>   	idx = rte_lcore_id();
>   
>   	/* last instance reserved for unregistered non-EAL threads */
> -	if (unlikely(idx == LCORE_ID_ANY))
> +	if (unlikely(idx == LCORE_ID_ANY)) {
>   		idx = RTE_MAX_LCORE;
> +		rte_spinlock_lock(&rte_rand_lock);

Non-EAL threads are very likely to be "regular" threads, which won't 
have a dedicated core all for themselves, and thus may well be preempted 
by the kernel. Such threads should not use spinlocks.

If a lock is to be added to achieve MT safety for parallel calls from 
unregistered non-EAL threads, it should be a regular mutex.

> +	}
>   
>   	return &rand_states[idx];
>   }
>   
> +static __rte_always_inline
> +void __rte_rand_put_state(struct rte_rand_state *state)
> +{
> +	if (state == &rand_states[RTE_MAX_LCORE])
> +		rte_spinlock_unlock(&rte_rand_lock);
> +}
> +
>   uint64_t
>   rte_rand(void)
>   {
>   	struct rte_rand_state *state;
> +	uint64_t res;
>   
>   	state = __rte_rand_get_state();
> +	res = __rte_rand_lfsr258(state);
> +	__rte_rand_put_state(state);
>   
> -	return __rte_rand_lfsr258(state);
> +	return res;
>   }
>   
>   uint64_t
> @@ -159,22 +175,24 @@ rte_rand_max(uint64_t upper_bound)
>   	/* Handle power-of-2 upper_bound as a special case, since it
>   	 * has no bias issues.
>   	 */
> -	if (unlikely(ones == 1))
> -		return __rte_rand_lfsr258(state) & (upper_bound - 1);
> -
> -	/* The approach to avoiding bias is to create a mask that
> -	 * stretches beyond the request value range, and up to the
> -	 * next power-of-2. In case the masked generated random value
> -	 * is equal to or greater than the upper bound, just discard
> -	 * the value and generate a new one.
> -	 */
> -
> -	leading_zeros = rte_clz64(upper_bound);
> -	mask >>= leading_zeros;
> -
> -	do {
> -		res = __rte_rand_lfsr258(state) & mask;
> -	} while (unlikely(res >= upper_bound));
> +	if (unlikely(ones == 1)) {
> +		res = __rte_rand_lfsr258(state) & (upper_bound - 1);
> +	} else {
> +		/* The approach to avoiding bias is to create a mask that
> +		 * stretches beyond the request value range, and up to the
> +		 * next power-of-2. In case the masked generated random value
> +		 * is equal to or greater than the upper bound, just discard
> +		 * the value and generate a new one.
> +		 */
> +
> +		leading_zeros = rte_clz64(upper_bound);
> +		mask >>= leading_zeros;
> +
> +		do {
> +			res = __rte_rand_lfsr258(state) & mask;
> +		} while (unlikely(res >= upper_bound));
> +	}
> +	__rte_rand_put_state(state);
>   
>   	return res;
>   }

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Stephen Hemminger]

On Fri, 8 Sep 2023 22:48:54 +0200
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> On 2023-09-07 17:24, Stephen Hemminger wrote:
> > Add missing locking so that if two non-EAL threads call rte_rand()
> > they will not corrupt the per-thread state.
> > 
> > Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")  
> 
> The API documentation clearly states that no MT safety guarantees are 
> given for unregistered non-EAL threads. So this patch doesn't fix anything.
> 
> rte_rand() is MT safe for *registered* non-EAL threads.

Reading the documentation, it only guarantees safety if registered.
We should add an otherwise clause rather than leaving case as undefined.

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Mattias Rönnblom]

On 2023-09-08 22:56, Stephen Hemminger wrote:
> On Fri, 8 Sep 2023 22:48:54 +0200
> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
>> On 2023-09-07 17:24, Stephen Hemminger wrote:
>>> Add missing locking so that if two non-EAL threads call rte_rand()
>>> they will not corrupt the per-thread state.
>>>
>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
>>
>> The API documentation clearly states that no MT safety guarantees are
>> given for unregistered non-EAL threads. So this patch doesn't fix anything.
>>
>> rte_rand() is MT safe for *registered* non-EAL threads.
> 
> Reading the documentation, it only guarantees safety if registered.
> We should add an otherwise clause rather than leaving case as undefined.

I agree. It is MT safe in case only single unregistered non-EAL thread 
uses the API (or multiple such threads, provided they are properly 
[externally] serialized).

"If called from EAL threads, registered non-EAL threads or a *single* 
unregistered non-EAL thread, this function is thread-safe. Multiple 
unregistered non-EAL threads may not safely call this function in 
parallel (i.e., must use external serialization)."

A lot of words, unfortunately. Maybe this is better:

"rte_xxx() is MT safe, with the exception it may not be called from 
multiple *unregistered* non-EAL threads in parallel."

Then of course comes the issue that nobody knows what is an registered 
non-EAL thread is. DPDK threading terminology is a big mess, so no wonder.

Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads

[Stephen Hemminger]

On Thu,  7 Sep 2023 08:24:56 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> Add missing locking so that if two non-EAL threads call rte_rand()
> they will not corrupt the per-thread state.
> 
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Drop this patch in favor of fixing the documentation and leaving
any thread issues up to application to sort out.