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.
>
> > +{
> > + 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);
