> 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 <navas...@microsoft.com> > > > > 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.
But it could just return ENOTSUP for Windows if 'attr' parameter is not NULL, no? > > > > > > +{ > > > + 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? No, I am talking about another case: suppose we allocate a structure (with a mutex) inside shared memory and plan to use it for inter-process communication. But with rte_thread_mutex_init() implementation actual mutex object will be allocated on process heap (private area) and wouldn't be accessible by other processes. As an example: struct shared { rte_thread_mutex lock; uint32_t val; }; ... struct shared *p = rte_zmalloc(NULL, sizeof(*p), RTE_CACHE_LINE_SIZE); rte_thread_mutex_init(&p->lock); Now p->lock is in shared memory, but actual mutex object: p->lock.mutex_id is within process private memory. > > > > > > + 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);