From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 273BD4254B; Fri, 8 Sep 2023 22:48:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8B334028A; Fri, 8 Sep 2023 22:48:58 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 815F440285 for ; Fri, 8 Sep 2023 22:48:57 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id D757D1E863 for ; Fri, 8 Sep 2023 22:48:56 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id D63B01EDAA; Fri, 8 Sep 2023 22:48:56 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-2.3 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -2.3 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 896F21EBCE; Fri, 8 Sep 2023 22:48:54 +0200 (CEST) Message-ID: <136fb174-ca02-577e-e690-f770a0328f10@lysator.liu.se> Date: Fri, 8 Sep 2023 22:48:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 Subject: Re: [PATCH v2 2/2] random: make rte_rand() thread safe for non-EAL threads To: Stephen Hemminger , dev@dpdk.org Cc: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , Bruce Richardson References: <20230906155302.82749-1-stephen@networkplumber.org> <20230907152456.20570-1-stephen@networkplumber.org> <20230907152456.20570-3-stephen@networkplumber.org> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <20230907152456.20570-3-stephen@networkplumber.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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 > #include > #include > +#include > #include > > 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; > }