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 DA437A034E; Wed, 9 Feb 2022 04:08:57 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6BF2B410F3; Wed, 9 Feb 2022 04:08:57 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 739B14067E for ; Wed, 9 Feb 2022 04:08:55 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1059) id AB1FE20B90F0; Tue, 8 Feb 2022 19:08:54 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AB1FE20B90F0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1644376134; bh=pG9QfwOxJHZws//BmHQhu6+GJ4bZE2GJKQhRuoMxDAw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UOmV0aEtljW0PB8qx5f7uPjyVcattx0H51MggrTmNfYEdDg8RyUzfPvyiMMIYz7Ae wXl7n3l0Bhp9iAG3lkiJULi3R6CP3sIOsktUv8Y9c/pMuOfw7NFxgM2+efLfUZFBpj 8RQTsdbahQ/mnPEIgv1wSgcSJbd13NOGIUY6pO0s= Date: Tue, 8 Feb 2022 19:08:54 -0800 From: Narcisa Ana Maria Vasile To: "Ananyev, Konstantin" Cc: "Richardson, Bruce" , "david.marchand@redhat.com" , "dev@dpdk.org" , "dmitry.kozliuk@gmail.com" , "dmitrym@microsoft.com" , "khot@microsoft.com" , "navasile@microsoft.com" , "ocardona@microsoft.com" , "Kadam, Pallavi" , "roretzla@microsoft.com" , "talshn@nvidia.com" , "thomas@monjalon.net" Subject: Re: [PATCH v18 8/8] eal: implement functions for mutex management Message-ID: <20220209030854.GB9377@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 Mon, Feb 07, 2022 at 04:02:54PM +0000, Ananyev, Konstantin wrote: > > Add functions for mutex init, destroy, lock, unlock, trylock. > > > > Windows does not have a static initializer. Initialization > > is only done through InitializeCriticalSection(). To overcome this, > > RTE_INIT_MUTEX macro is added to replace static initialization > > of mutexes. The macro calls rte_thread_mutex_init(). > > > > Add unit tests to verify that the mutex correctly locks/unlocks > > and protects the data. Check both static and dynamic mutexes. > > Signed-off-by: Narcisa Vasile > > Few comments from me below. > I am not sure was such approach already discussed, > if so - apologies for repetition. > No worries, I appreciate your review! > > --- > > app/test/test_threads.c | 106 +++++++++++++++++++++++++++++++++++ > > lib/eal/common/rte_thread.c | 69 +++++++++++++++++++++++ > > lib/eal/include/rte_thread.h | 85 ++++++++++++++++++++++++++++ > > lib/eal/version.map | 5 ++ > > lib/eal/windows/rte_thread.c | 64 +++++++++++++++++++++ > > 5 files changed, 329 insertions(+) > > > > }; > > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c > > index d30a8a7ca3..4a9a1b6e07 100644 > > --- a/lib/eal/common/rte_thread.c > > +++ b/lib/eal/common/rte_thread.c > > @@ -309,6 +309,75 @@ rte_thread_detach(rte_thread_t thread_id) > > return pthread_detach((pthread_t)thread_id.opaque_id); > > } > > > > +int > > +rte_thread_mutex_init(rte_thread_mutex *mutex) > > Don't we need some sort of mutex_attr here too? > To be able to create PROCESS_SHARED mutexes? Attributes are tricky to implement on Windows. In order to not overcomplicate this patchset and since the drivers that need them don't compile on Windows anyway, I decided to omit them from this patchset. In the future, after enabling the new thread API, we can consider implementing them as well. > > > +{ > > + int ret = 0; > > + pthread_mutex_t *m = NULL; > > + > > + RTE_VERIFY(mutex != NULL); > > + > > + m = calloc(1, sizeof(*m)); > > But is that what we really want for the mutexes? > It means actual mutex will always be allocated on process heap, > away from the data it is supposed to guard. > Even if we'll put performance considerations away, > that wouldn't work for MP case. > Is that considered as ok? Are you refering to the fact that all mutexes will be dynamically allocated, due to the static intializer calling _mutex_init() in the background? Why wouldn't it work in the MP case? > > > + if (m == NULL) { > > + RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n"); > > + ret = ENOMEM; > > + goto cleanup; > > + } > > + > > + > > + return ret; > > +} > > + return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);