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

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

[Ananyev, Konstantin]

> 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. 

> ---
>  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?

> +{
> +	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?

> +	if (m == NULL) {
> +		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
> +		ret = ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	ret = pthread_mutex_init(m, NULL);
> +	if (ret != 0) {
> +		RTE_LOG(DEBUG, EAL, "Failed to init mutex. ret = %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	mutex->mutex_id = m;
> +	m = NULL;
> +
> +cleanup:
> +	free(m);
> +	return ret;
> +}
> +
> +int
> +rte_thread_mutex_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_lock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_unlock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_unlock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
> +{
> +	RTE_VERIFY(mutex != NULL);
> +
> +	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);
> +}
> +
> +int
> +rte_thread_mutex_destroy(rte_thread_mutex *mutex)
> +{
> +	int ret = 0;
> +	RTE_VERIFY(mutex != NULL);
> +
> +	ret = pthread_mutex_destroy((pthread_mutex_t *)mutex->mutex_id);
> +	if (ret != 0)
> +		RTE_LOG(DEBUG, EAL, "Unable to destroy mutex, ret = %d\n", ret);
> +
> +	free(mutex->mutex_id);
> +	mutex->mutex_id = NULL;
> +
> +	return ret;
> +}
> +
>  int
>  rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
>  {
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 7c84e32988..09a5fd8add 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -54,6 +54,25 @@ typedef struct {
> 
>  #endif /* RTE_HAS_CPUSET */
> 
> +#define RTE_DECLARE_MUTEX(private_lock)          rte_thread_mutex private_lock
> +
> +#define RTE_DEFINE_MUTEX(private_lock)\
> +RTE_INIT(__rte_ ## private_lock ## _init)\
> +{\
> +	RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
> +}
> +
> +#define RTE_INIT_MUTEX(private_lock)\
> +static RTE_DECLARE_MUTEX(private_lock);\
> +RTE_DEFINE_MUTEX(private_lock)

Hmm, but that way we can't use  RTE_INIT_MUTEX()
within functions, right?

> +
> +/**
> + * Thread mutex representation.
> + */
> +typedef struct rte_thread_mutex_tag {
> +	void *mutex_id;  /**< mutex identifier */
> +} rte_thread_mutex;

I wonder can't we have something like that instead:

for posix:
typedef pthread_mutex_t rte_thread_mutex_t;
for windows: 
typedef struct rte_thread_mutex {
	int initialized;
	CRITICAL_SECTION cs;
}  rte_thread_mutex_t;

Then for posix:
#define RTE_INIT_MUTEX(mx) do {\
	*(mx) = PTHREAD_MUTEX_INITIALIZER; \
} while(0)

#define RTE_DESTROY_MUTEX(mx)	do {} while (0); /*empty */

For windows:
#define RTE_INIT_MUTEX(mx) do {\
	If ((mx)->initialized == 0) {
		InitializeCriticalSection((mx)->cs);
		(mx)->initialized = 1;
	}
} while (0)

#define RTE_DESTROY_MUTEX(mx)	do {
	if ((mx)->initialized != 0) { \
		DeleteCriticalSection((mx)->cs);
	}
} while (0)

That way we'll keep ability for static initialization on posix systems,
and would avoid need to allocate actual mutex object from the heap. 

>  /**
>   * Returned by rte_thread_barrier_wait() when call is successful.
>   */
> @@ -314,6 +333,72 @@ __rte_experimental
>  int rte_thread_set_priority(rte_thread_t thread_id,
>  		enum rte_thread_priority priority);
> 
> +/**
> + * Initializes a mutex.
> + *
> + * @param mutex
> + *    The mutex to be initialized.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_init(rte_thread_mutex *mutex);
> +
> +/**
> + * Locks a mutex.
> + *
> + * @param mutex
> + *    The mutex to be locked.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_lock(rte_thread_mutex *mutex);
> +
> +/**
> + * Unlocks a mutex.
> + *
> + * @param mutex
> + *    The mutex to be unlocked.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_unlock(rte_thread_mutex *mutex);
> +
> +/**
> + * Tries to lock a mutex.If the mutex is already held by a different thread,
> + * the function returns without blocking.
> + *
> + * @param mutex
> + *    The mutex that will be acquired, if not already locked.
> + *
> + * @return
> + *   On success, if the mutex is acquired, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_try_lock(rte_thread_mutex *mutex);
> +
> +/**
> + * Releases all resources associated with a mutex.
> + *
> + * @param mutex
> + *    The mutex to be uninitialized.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_mutex_destroy(rte_thread_mutex *mutex);
> +
>  /**
>   * Initializes a synchronization barrier.
>   *

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

[Ananyev, Konstantin]

> > +
> > +/**
> > + * Thread mutex representation.
> > + */
> > +typedef struct rte_thread_mutex_tag {
> > +	void *mutex_id;  /**< mutex identifier */
> > +} rte_thread_mutex;
> 
> I wonder can't we have something like that instead:
> 
> for posix:
> typedef pthread_mutex_t rte_thread_mutex_t;
> for windows:
> typedef struct rte_thread_mutex {
> 	int initialized;
> 	CRITICAL_SECTION cs;
> }  rte_thread_mutex_t;
> 
> Then for posix:
> #define RTE_INIT_MUTEX(mx) do {\
> 	*(mx) = PTHREAD_MUTEX_INITIALIZER; \
> } while(0)
> 
> #define RTE_DESTROY_MUTEX(mx)	do {} while (0); /*empty */
> 
> For windows:
> #define RTE_INIT_MUTEX(mx) do {\
> 	If ((mx)->initialized == 0) {
> 		InitializeCriticalSection((mx)->cs);
> 		(mx)->initialized = 1;
> 	}
> } while (0)
> 
> #define RTE_DESTROY_MUTEX(mx)	do {
> 	if ((mx)->initialized != 0) { \
> 		DeleteCriticalSection((mx)->cs);
> 	}
> } while (0)
> 

Actually, please scrap that comment.
Obviously it wouldn't work for static variables, 
and doesn't make much sense.
Though few thoughts remain:
for posix we probably don't need an indirection and
rte_thread_mutex can be just typedef of pthread_mutex_t.
also for posix we don't need RTE_INIT constructor for each
static mutex initialization.
Something like:
#define RTE_STATIC_INITIALIZED_MUTEX(mx) \
	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
should work, I think.
Konstantin

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

[Narcisa Ana Maria Vasile]

On Tue, Feb 08, 2022 at 02:21:49AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > > +
> > > +/**
> > > + * Thread mutex representation.
> > 
> 
> Actually, please scrap that comment.
> Obviously it wouldn't work for static variables, 
> and doesn't make much sense.
> Though few thoughts remain:
> for posix we probably don't need an indirection and
> rte_thread_mutex can be just typedef of pthread_mutex_t.
> also for posix we don't need RTE_INIT constructor for each
> static mutex initialization.
> Something like:
> #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> should work, I think.
> Konstantin

Thank you for reviewing, Konstantin!
Some context for the current representation of mutex
can be found in v9, patch 7/10 of this patchset.

Originally we've typedef'ed the pthread_mutex_t on POSIX, just
like you are suggesting here.
However, on Windows there's no static initializer similar to the pthread
one. Still, we want ABI compatibility and same thread behavior between
platforms. The most elegant solution we found was the current representation,
as suggested by Dmitry K.

I will address your other comments on the other thread.

Link to v9: http://patchwork.dpdk.org/project/dpdk/patch/1622850274-6946-8-git-send-email-navasile@linux.microsoft.com/
> 
> 

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

[Narcisa Ana Maria Vasile]

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

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

[Ananyev, Konstantin]

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

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

[Ananyev, Konstantin]

> > Actually, please scrap that comment.
> > Obviously it wouldn't work for static variables,
> > and doesn't make much sense.
> > Though few thoughts remain:
> > for posix we probably don't need an indirection and
> > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > also for posix we don't need RTE_INIT constructor for each
> > static mutex initialization.
> > Something like:
> > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > should work, I think.
> > Konstantin
> 
> Thank you for reviewing, Konstantin!
> Some context for the current representation of mutex
> can be found in v9, patch 7/10 of this patchset.
> 
> Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> like you are suggesting here.
> However, on Windows there's no static initializer similar to the pthread
> one. Still, we want ABI compatibility and same thread behavior between
> platforms. The most elegant solution we found was the current representation,
> as suggested by Dmitry K.

Yes, I agree it is a problem with Windows for static initializer.
But why we can't have different structs typedef for mutex 
for posix and windows platforms?
On posix it would be:

typedef pthread_mutex_t rte_thread_mutex_t;
#define RTE_STATIC_INITIALIZED_MUTEX(mx)   rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER 

On windows it could be what Dimitry suggested:
 
typedef struct rte_thread_mutex {
        void *mutex_id;  /**< mutex identifier */
} rte_thread_mutex_t;

#define RTE_STATIC_INITIALIZED_MUTEX(private_lock)   \
rte_thread_mutex_t private_lock; \
RTE_INIT(__rte_ ## private_lock ## _init)\
{\
        RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
}

API would remain the same, though it would be different underneath.
Yes, on Windows rte_thread_mutex still wouldn't work for MP,
but that's the same as with current design.
 
> I will address your other comments on the other thread.
> 
> Link to v9: http://patchwork.dpdk.org/project/dpdk/patch/1622850274-6946-8-git-send-email-navasile@linux.microsoft.com/

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

[Dmitry Kozlyuk]

2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > Actually, please scrap that comment.
> > > Obviously it wouldn't work for static variables,
> > > and doesn't make much sense.
> > > Though few thoughts remain:
> > > for posix we probably don't need an indirection and
> > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > also for posix we don't need RTE_INIT constructor for each
> > > static mutex initialization.
> > > Something like:
> > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > should work, I think.
> > > Konstantin  
> > 
> > Thank you for reviewing, Konstantin!
> > Some context for the current representation of mutex
> > can be found in v9, patch 7/10 of this patchset.
> > 
> > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > like you are suggesting here.
> > However, on Windows there's no static initializer similar to the pthread
> > one. Still, we want ABI compatibility and same thread behavior between
> > platforms. The most elegant solution we found was the current representation,
> > as suggested by Dmitry K.  
> 
> Yes, I agree it is a problem with Windows for static initializer.
> But why we can't have different structs typedef for mutex 
> for posix and windows platforms?

Yes, I agree that having different mutex types on *nix and Windows
is a great idea. It will avoid ABI change for *nix
and will guarantee no performance impact.

Maybe wrap pthread_mutex_t into a struct to have a distinct type
and to force using only DPDK API with it?

[...]
> Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> but that's the same as with current design.

MP support is not planned for Windows and it is unknown if it ever will be,
so it's not an issue.
Data location is.
The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
(akin to pthread_mutex_t) is to avoid including Windows headers
into DPDK public headers, because Windows headers can break user code
by some macros they define.
Maybe instead of a pointer it could be an opaque array:

	#define RTE_PTHREAD_MUTEX_SIZE 40

	struct rte_pthread_mutex_t {
		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
	};

where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
Win32 ABI is remarkably stable, I don't think this constant will ever change,
it would break all the Windows user space.
Naty, DmitryM, Tyler, what do you think?

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

[Dmitry Kozlyuk]

2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:
> 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > > Actually, please scrap that comment.
> > > > Obviously it wouldn't work for static variables,
> > > > and doesn't make much sense.
> > > > Though few thoughts remain:
> > > > for posix we probably don't need an indirection and
> > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > also for posix we don't need RTE_INIT constructor for each
> > > > static mutex initialization.
> > > > Something like:
> > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > should work, I think.
> > > > Konstantin    
> > > 
> > > Thank you for reviewing, Konstantin!
> > > Some context for the current representation of mutex
> > > can be found in v9, patch 7/10 of this patchset.
> > > 
> > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > like you are suggesting here.
> > > However, on Windows there's no static initializer similar to the pthread
> > > one. Still, we want ABI compatibility and same thread behavior between
> > > platforms. The most elegant solution we found was the current representation,
> > > as suggested by Dmitry K.    
> > 
> > Yes, I agree it is a problem with Windows for static initializer.
> > But why we can't have different structs typedef for mutex 
> > for posix and windows platforms?  
> 
> Yes, I agree that having different mutex types on *nix and Windows
> is a great idea. It will avoid ABI change for *nix
> and will guarantee no performance impact.
> 
> Maybe wrap pthread_mutex_t into a struct to have a distinct type
> and to force using only DPDK API with it?
> 
> [...]
> > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > but that's the same as with current design.  
> 
> MP support is not planned for Windows and it is unknown if it ever will be,
> so it's not an issue.
> Data location is.
> The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> (akin to pthread_mutex_t) is to avoid including Windows headers
> into DPDK public headers, because Windows headers can break user code
> by some macros they define.
> Maybe instead of a pointer it could be an opaque array:
> 
> 	#define RTE_PTHREAD_MUTEX_SIZE 40
> 
> 	struct rte_pthread_mutex_t {
> 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> 	};
> 
> where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> Win32 ABI is remarkably stable, I don't think this constant will ever change,
> it would break all the Windows user space.
> Naty, DmitryM, Tyler, what do you think?

Conclusion from offline call: yes, this is OK to do so.

However, DmitryM suggested using Slim Reader-Writer lock (SRW):
https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
instead of CRITICAL_SECTION.
It seems to be a much better option:

* sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
  same as sizeof(pthread_mutex_t) on a typical Linux.
  Layout of data structures containing rte_thread_mutex_t
  can be the same on Windows and Unix,
  which simplifies design and promises similar less performance differences.

* Can be taken by multiple readers and one writer,
  which is semantically similar to pthread_mutex_t
  (CRITICAL_SECTION can only be taken by a single thread).

Technically it can be a "typedef uintptr_t" or a structure wrapping it.

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

[Ananyev, Konstantin]

Hi Dmitry,

> 2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:
> > 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:
> > > > > Actually, please scrap that comment.
> > > > > Obviously it wouldn't work for static variables,
> > > > > and doesn't make much sense.
> > > > > Though few thoughts remain:
> > > > > for posix we probably don't need an indirection and
> > > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > > also for posix we don't need RTE_INIT constructor for each
> > > > > static mutex initialization.
> > > > > Something like:
> > > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > > should work, I think.
> > > > > Konstantin
> > > >
> > > > Thank you for reviewing, Konstantin!
> > > > Some context for the current representation of mutex
> > > > can be found in v9, patch 7/10 of this patchset.
> > > >
> > > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > > like you are suggesting here.
> > > > However, on Windows there's no static initializer similar to the pthread
> > > > one. Still, we want ABI compatibility and same thread behavior between
> > > > platforms. The most elegant solution we found was the current representation,
> > > > as suggested by Dmitry K.
> > >
> > > Yes, I agree it is a problem with Windows for static initializer.
> > > But why we can't have different structs typedef for mutex
> > > for posix and windows platforms?
> >
> > Yes, I agree that having different mutex types on *nix and Windows
> > is a great idea. It will avoid ABI change for *nix
> > and will guarantee no performance impact.
> >
> > Maybe wrap pthread_mutex_t into a struct to have a distinct type
> > and to force using only DPDK API with it?
> >
> > [...]
> > > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > > but that's the same as with current design.
> >
> > MP support is not planned for Windows and it is unknown if it ever will be,
> > so it's not an issue.
> > Data location is.
> > The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> > (akin to pthread_mutex_t) is to avoid including Windows headers
> > into DPDK public headers, because Windows headers can break user code
> > by some macros they define.
> > Maybe instead of a pointer it could be an opaque array:
> >
> > 	#define RTE_PTHREAD_MUTEX_SIZE 40
> >
> > 	struct rte_pthread_mutex_t {
> > 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> > 	};
> >
> > where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> > Win32 ABI is remarkably stable, I don't think this constant will ever change,
> > it would break all the Windows user space.
> > Naty, DmitryM, Tyler, what do you think?
> 
> Conclusion from offline call: yes, this is OK to do so.
> 
> However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> instead of CRITICAL_SECTION.
> It seems to be a much better option:
> 
> * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
>   same as sizeof(pthread_mutex_t) on a typical Linux.
>   Layout of data structures containing rte_thread_mutex_t
>   can be the same on Windows and Unix,
>   which simplifies design and promises similar less performance differences.
> 
> * Can be taken by multiple readers and one writer,
>   which is semantically similar to pthread_mutex_t

Not sure I understand you here:
pthread_mutex provides only mutually-exclusive lock semantics.
For RW lock there exists: pthread_rwlock_t.
Off-course you can use rwlock fo exclusive locking too -
if all callers will use only writer locks, but usually that's no point to do that -
mutexes are simpler and faster.
That's for posix-like systems, don't know much about Windows environment :)

>   (CRITICAL_SECTION can only be taken by a single thread).
> 
> Technically it can be a "typedef uintptr_t" or a structure wrapping it.

Again can't say much about Windows, but pthread_mutex_t
can (and is) bigger then then 8 bytes. 

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

[Stephen Hemminger]

On Thu, 24 Feb 2022 17:29:03 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> Hi Dmitry,
> 
> > 2022-02-21 00:56 (UTC+0300), Dmitry Kozlyuk:  
> > > 2022-02-09 13:57 (UTC+0000), Ananyev, Konstantin:  
> > > > > > Actually, please scrap that comment.
> > > > > > Obviously it wouldn't work for static variables,
> > > > > > and doesn't make much sense.
> > > > > > Though few thoughts remain:
> > > > > > for posix we probably don't need an indirection and
> > > > > > rte_thread_mutex can be just typedef of pthread_mutex_t.
> > > > > > also for posix we don't need RTE_INIT constructor for each
> > > > > > static mutex initialization.
> > > > > > Something like:
> > > > > > #define RTE_STATIC_INITIALIZED_MUTEX(mx) \
> > > > > > 	rte_thread_mutex_t mx = PTHREAD_MUTEX_INITIALIZER
> > > > > > should work, I think.
> > > > > > Konstantin  
> > > > >
> > > > > Thank you for reviewing, Konstantin!
> > > > > Some context for the current representation of mutex
> > > > > can be found in v9, patch 7/10 of this patchset.
> > > > >
> > > > > Originally we've typedef'ed the pthread_mutex_t on POSIX, just
> > > > > like you are suggesting here.
> > > > > However, on Windows there's no static initializer similar to the pthread
> > > > > one. Still, we want ABI compatibility and same thread behavior between
> > > > > platforms. The most elegant solution we found was the current representation,
> > > > > as suggested by Dmitry K.  
> > > >
> > > > Yes, I agree it is a problem with Windows for static initializer.
> > > > But why we can't have different structs typedef for mutex
> > > > for posix and windows platforms?  
> > >
> > > Yes, I agree that having different mutex types on *nix and Windows
> > > is a great idea. It will avoid ABI change for *nix
> > > and will guarantee no performance impact.
> > >
> > > Maybe wrap pthread_mutex_t into a struct to have a distinct type
> > > and to force using only DPDK API with it?
> > >
> > > [...]  
> > > > Yes, on Windows rte_thread_mutex still wouldn't work for MP,
> > > > but that's the same as with current design.  
> > >
> > > MP support is not planned for Windows and it is unknown if it ever will be,
> > > so it's not an issue.
> > > Data location is.
> > > The reason rte_thread_mutex_t is not a typedef of CRITICAL_SECTION
> > > (akin to pthread_mutex_t) is to avoid including Windows headers
> > > into DPDK public headers, because Windows headers can break user code
> > > by some macros they define.
> > > Maybe instead of a pointer it could be an opaque array:
> > >
> > > 	#define RTE_PTHREAD_MUTEX_SIZE 40
> > >
> > > 	struct rte_pthread_mutex_t {
> > > 		uint8_t opaque[RTE_PTHREAD_MUTEX_SIZE];
> > > 	};
> > >
> > > where RTE_PTHREAD_MUTEX_SIZE is actually sizeof(CRITICAL_SECTION).
> > > Win32 ABI is remarkably stable, I don't think this constant will ever change,
> > > it would break all the Windows user space.
> > > Naty, DmitryM, Tyler, what do you think?  
> > 
> > Conclusion from offline call: yes, this is OK to do so.
> > 
> > However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> > https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> > instead of CRITICAL_SECTION.
> > It seems to be a much better option:
> > 
> > * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
> >   same as sizeof(pthread_mutex_t) on a typical Linux.
> >   Layout of data structures containing rte_thread_mutex_t
> >   can be the same on Windows and Unix,
> >   which simplifies design and promises similar less performance differences.
> > 
> > * Can be taken by multiple readers and one writer,
> >   which is semantically similar to pthread_mutex_t  
> 
> Not sure I understand you here:
> pthread_mutex provides only mutually-exclusive lock semantics.
> For RW lock there exists: pthread_rwlock_t.
> Off-course you can use rwlock fo exclusive locking too -
> if all callers will use only writer locks, but usually that's no point to do that -
> mutexes are simpler and faster.
> That's for posix-like systems, don't know much about Windows environment :)
> 
> >   (CRITICAL_SECTION can only be taken by a single thread).
> > 
> > Technically it can be a "typedef uintptr_t" or a structure wrapping it.  
> 
> Again can't say much about Windows, but pthread_mutex_t
> can (and is) bigger then then 8 bytes. 
> 
> 


There seems to be some confusion here:
   pthread_mutex put thread to sleep if contended and on linux are built on the futex system call.
   pthread_rwlock are the reader/writer versions of these.

The DPDK has primitives for multiple types of locks: spinlock, rwlock, ticketlock, pflock, etc
   these are build using atomic primitives (no syscall).
   these are platform independent
   these spin if contended

Not sure about Windows, but it looks like slim rwlocks came from Windows NT and are an implementation
of the same kind of spinning lock DPDK already has.

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

[Dmitry Kozlyuk]

Hi Konstantin,

2022-02-24 17:29 (UTC+0000), Ananyev, Konstantin:
[...]
> > However, DmitryM suggested using Slim Reader-Writer lock (SRW):
> > https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
> > instead of CRITICAL_SECTION.
> > It seems to be a much better option:
> > 
> > * sizeof(SRWLOCK) == 8 (technically "size of a pointer"),
> >   same as sizeof(pthread_mutex_t) on a typical Linux.
> >   Layout of data structures containing rte_thread_mutex_t
> >   can be the same on Windows and Unix,
> >   which simplifies design and promises similar less performance differences.
> > 
> > * Can be taken by multiple readers and one writer,
> >   which is semantically similar to pthread_mutex_t  
> 
> Not sure I understand you here:
> pthread_mutex provides only mutually-exclusive lock semantics.
> For RW lock there exists: pthread_rwlock_t.
> Off-course you can use rwlock fo exclusive locking too -
> if all callers will use only writer locks, but usually that's no point to do that -
> mutexes are simpler and faster.
> That's for posix-like systems, don't know much about Windows environment :)

It is different on Windows.
Multiple sources claim SRW lock is faster than CRITICAL_SECTION
even when used only for exclusive locking.
SRW locks do not support recursive locking,
while CRITICAL_SECTION is always recursive.
I see PTHREAD_MUTEX_RECURSIVE used in net/failsafe and raw/ifpga.
CRITICAL_SECTION also keeps debug information to analyze deadlocks
(can't say much here, never used this feature).

> > Technically it can be a "typedef uintptr_t" or a structure wrapping it.  
> 
> Again can't say much about Windows, but pthread_mutex_t
> can (and is) bigger then then 8 bytes. 

My bad, I measured incorrectly: sizeof(pthread_mutex_t) is 40 on my system.

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

[Dmitry Kozlyuk]

Hi Stephen,

2022-02-24 09:44 (UTC-0800), Stephen Hemminger:
> There seems to be some confusion here:
>    pthread_mutex put thread to sleep if contended and on linux are built on the futex system call.
>    pthread_rwlock are the reader/writer versions of these.
> 
> The DPDK has primitives for multiple types of locks: spinlock, rwlock, ticketlock, pflock, etc
>    these are build using atomic primitives (no syscall).
>    these are platform independent
>    these spin if contended
> 
> Not sure about Windows, but it looks like slim rwlocks came from Windows NT and are an implementation
> of the same kind of spinning lock DPDK already has.

Both CRITICAL_SECTION and SRW lock spin shortly before going into kernel,
but both do this eventually---SRW lock is not a purely user space primitive.

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

[Narcisa Ana Maria Vasile]

On Wed, Nov 10, 2021 at 05:33:45PM -0800, Narcisa Ana Maria Vasile wrote:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> 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>
> ---

Hi Bruce R., Stephen H., I've followed Bruce's suggestion and changed this patchset
to return ENOTSUP for realtime priority on Linux. Can you please take a look at
the latest version of the patchset?
Let me know if it needs any changes or if it's ready to be merged. Thank you!

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

[Narcisa Ana Maria Vasile]

From: Narcisa Vasile <navasile@microsoft.com>

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>
---
 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/app/test/test_threads.c b/app/test/test_threads.c
index 00f604ab7e..91155a04e3 100644
--- a/app/test/test_threads.c
+++ b/app/test/test_threads.c
@@ -243,6 +243,110 @@ test_thread_barrier(void)
 	return 0;
 }
 
+RTE_INIT_MUTEX(static_mutex);
+
+struct mutex_loop_args {
+	rte_thread_barrier *barrier;
+	rte_thread_mutex *mutex;
+	unsigned long result_A;
+	unsigned long result_B;
+};
+
+static void *
+thread_loop_mutex_B(void *arg)
+{
+	struct mutex_loop_args *args = arg;
+
+	if (rte_thread_mutex_try_lock(args->mutex) == 0) {
+		rte_thread_barrier_wait(args->barrier);
+		rte_thread_mutex_unlock(args->mutex);
+		args->result_B = 1;
+	} else {
+		rte_thread_barrier_wait(args->barrier);
+		args->result_B = 2;
+	}
+
+	return NULL;
+}
+
+static void *
+thread_loop_mutex_A(void *arg)
+{
+	struct mutex_loop_args *args = arg;
+
+	if (rte_thread_mutex_try_lock(args->mutex) != 0) {
+		rte_thread_barrier_wait(args->barrier);
+		args->result_A = 2;
+	} else {
+		rte_thread_barrier_wait(args->barrier);
+		rte_thread_mutex_unlock(args->mutex);
+		args->result_A = 1;
+	}
+
+	return NULL;
+}
+
+static int
+test_thread_mutex(rte_thread_mutex *pmutex)
+{
+	rte_thread_t thread_A;
+	rte_thread_t thread_B;
+	rte_thread_mutex mutex;
+	rte_thread_barrier barrier;
+	struct mutex_loop_args args;
+	int ret = 0;
+
+	/* If mutex is not statically initialized */
+	if (pmutex == NULL) {
+		ret = rte_thread_mutex_init(&mutex);
+		RTE_TEST_ASSERT(ret == 0, "Failed to initialize mutex!");
+	} else
+		mutex = *pmutex;
+
+	ret = rte_thread_barrier_init(&barrier, 2);
+	RTE_TEST_ASSERT(ret == 0, "Failed to initialize barrier!");
+
+	args.mutex = &mutex;
+	args.barrier = &barrier;
+
+	ret = rte_thread_create(&thread_A, NULL, thread_loop_mutex_A, &args);
+	RTE_TEST_ASSERT(ret == 0, "Failed to create thread!");
+
+	ret = rte_thread_create(&thread_B, NULL, thread_loop_mutex_B, &args);
+	RTE_TEST_ASSERT(ret == 0, "Failed to create thread!");
+
+	ret = rte_thread_join(thread_A, NULL);
+	RTE_TEST_ASSERT(ret == 0, "Failed to join thread!");
+
+	ret = rte_thread_join(thread_B, NULL);
+	RTE_TEST_ASSERT(ret == 0, "Failed to join thread!");
+
+	RTE_TEST_ASSERT(args.result_A != args.result_B, "Mutex failed to be acquired or was acquired by both threads!");
+
+	/* Destroy if dynamically initialized */
+	if (pmutex == NULL) {
+		ret = rte_thread_mutex_destroy(&mutex);
+		RTE_TEST_ASSERT(ret == 0, "Failed to destroy mutex!");
+	}
+
+	ret = rte_thread_barrier_destroy(&barrier);
+	RTE_TEST_ASSERT(ret == 0, "Failed to destroy barrier!");
+
+	return ret;
+}
+
+static int
+test_thread_mutex_static(void)
+{
+	return test_thread_mutex(&static_mutex);
+}
+
+static int
+test_thread_mutex_dynamic(void)
+{
+	return test_thread_mutex(NULL);
+}
+
 static struct unit_test_suite threads_test_suite = {
 	.suite_name = "threads autotest",
 	.setup = NULL,
@@ -253,6 +357,8 @@ static struct unit_test_suite threads_test_suite = {
 			TEST_CASE(test_thread_attributes_priority),
 			TEST_CASE(test_thread_detach),
 			TEST_CASE(test_thread_barrier),
+			TEST_CASE(test_thread_mutex_static),
+			TEST_CASE(test_thread_mutex_dynamic),
 			TEST_CASES_END()
 	}
 };
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)
+{
+	int ret = 0;
+	pthread_mutex_t *m = NULL;
+
+	RTE_VERIFY(mutex != NULL);
+
+	m = calloc(1, sizeof(*m));
+	if (m == NULL) {
+		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
+		ret = ENOMEM;
+		goto cleanup;
+	}
+
+	ret = pthread_mutex_init(m, NULL);
+	if (ret != 0) {
+		RTE_LOG(DEBUG, EAL, "Failed to init mutex. ret = %d\n", ret);
+		goto cleanup;
+	}
+
+	mutex->mutex_id = m;
+	m = NULL;
+
+cleanup:
+	free(m);
+	return ret;
+}
+
+int
+rte_thread_mutex_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_lock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_unlock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	return pthread_mutex_trylock((pthread_mutex_t *)mutex->mutex_id);
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex *mutex)
+{
+	int ret = 0;
+	RTE_VERIFY(mutex != NULL);
+
+	ret = pthread_mutex_destroy((pthread_mutex_t *)mutex->mutex_id);
+	if (ret != 0)
+		RTE_LOG(DEBUG, EAL, "Unable to destroy mutex, ret = %d\n", ret);
+
+	free(mutex->mutex_id);
+	mutex->mutex_id = NULL;
+
+	return ret;
+}
+
 int
 rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
 {
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index 7c84e32988..09a5fd8add 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -54,6 +54,25 @@ typedef struct {
 
 #endif /* RTE_HAS_CPUSET */
 
+#define RTE_DECLARE_MUTEX(private_lock)          rte_thread_mutex private_lock
+
+#define RTE_DEFINE_MUTEX(private_lock)\
+RTE_INIT(__rte_ ## private_lock ## _init)\
+{\
+	RTE_VERIFY(rte_thread_mutex_init(&private_lock) == 0);\
+}
+
+#define RTE_INIT_MUTEX(private_lock)\
+static RTE_DECLARE_MUTEX(private_lock);\
+RTE_DEFINE_MUTEX(private_lock)
+
+/**
+ * Thread mutex representation.
+ */
+typedef struct rte_thread_mutex_tag {
+	void *mutex_id;  /**< mutex identifier */
+} rte_thread_mutex;
+
 /**
  * Returned by rte_thread_barrier_wait() when call is successful.
  */
@@ -314,6 +333,72 @@ __rte_experimental
 int rte_thread_set_priority(rte_thread_t thread_id,
 		enum rte_thread_priority priority);
 
+/**
+ * Initializes a mutex.
+ *
+ * @param mutex
+ *    The mutex to be initialized.
+ *
+ * @return
+ *   On success, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_init(rte_thread_mutex *mutex);
+
+/**
+ * Locks a mutex.
+ *
+ * @param mutex
+ *    The mutex to be locked.
+ *
+ * @return
+ *   On success, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_lock(rte_thread_mutex *mutex);
+
+/**
+ * Unlocks a mutex.
+ *
+ * @param mutex
+ *    The mutex to be unlocked.
+ *
+ * @return
+ *   On success, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_unlock(rte_thread_mutex *mutex);
+
+/**
+ * Tries to lock a mutex.If the mutex is already held by a different thread,
+ * the function returns without blocking.
+ *
+ * @param mutex
+ *    The mutex that will be acquired, if not already locked.
+ *
+ * @return
+ *   On success, if the mutex is acquired, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_try_lock(rte_thread_mutex *mutex);
+
+/**
+ * Releases all resources associated with a mutex.
+ *
+ * @param mutex
+ *    The mutex to be uninitialized.
+ *
+ * @return
+ *   On success, return 0.
+ *   On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_mutex_destroy(rte_thread_mutex *mutex);
+
 /**
  * Initializes a synchronization barrier.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 06e5f82da2..e80eea4316 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -431,6 +431,11 @@ EXPERIMENTAL {
 	rte_thread_barrier_wait;
 	rte_thread_barrier_destroy;
 	rte_thread_get_affinity_by_id;
+	rte_thread_mutex_init;
+	rte_thread_mutex_lock;
+	rte_thread_mutex_unlock;
+	rte_thread_mutex_try_lock;
+	rte_thread_mutex_destroy;
 	rte_thread_set_affinity_by_id;
 	rte_thread_get_priority;
 	rte_thread_set_priority;
diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
index 3f72bbf716..11b4863fe8 100644
--- a/lib/eal/windows/rte_thread.c
+++ b/lib/eal/windows/rte_thread.c
@@ -504,6 +504,70 @@ rte_thread_detach(rte_thread_t thread_id)
 	return 0;
 }
 
+int
+rte_thread_mutex_init(rte_thread_mutex *mutex)
+{
+	int ret = 0;
+	CRITICAL_SECTION *m = NULL;
+
+	RTE_VERIFY(mutex != NULL);
+
+	m = calloc(1, sizeof(*m));
+	if (m == NULL) {
+		RTE_LOG(DEBUG, EAL, "Unable to initialize mutex. Insufficient memory!\n");
+		ret = ENOMEM;
+		goto cleanup;
+	}
+
+	InitializeCriticalSection(m);
+	mutex->mutex_id = m;
+	m = NULL;
+
+cleanup:
+	return ret;
+}
+
+int
+rte_thread_mutex_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	EnterCriticalSection(mutex->mutex_id);
+	return 0;
+}
+
+int
+rte_thread_mutex_unlock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	LeaveCriticalSection(mutex->mutex_id);
+	return 0;
+}
+
+int
+rte_thread_mutex_try_lock(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	if (TryEnterCriticalSection(mutex->mutex_id) != 0)
+		return 0;
+
+	return EBUSY;
+}
+
+int
+rte_thread_mutex_destroy(rte_thread_mutex *mutex)
+{
+	RTE_VERIFY(mutex != NULL);
+
+	DeleteCriticalSection(mutex->mutex_id);
+	free(mutex->mutex_id);
+	mutex->mutex_id = NULL;
+
+	return 0;
+}
+
 int
 rte_thread_barrier_init(rte_thread_barrier *barrier, int count)
 {
-- 
2.31.0.vfs.0.1