Re: [PATCH v18 8/8] eal: implement functions for mutex management

[Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>]

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 <navasile@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);