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

["Ananyev, Konstantin" <konstantin.ananyev@intel.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.

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